[MC][COFF][AArch64] Avoid incorrect IMAGE_REL_ARM64_BRANCH26 relocations.

[hjyamauchi]

Summary

For a b/bl instruction that branches a temporary symbol (private assembly label), an IMAGE_REL_ARM64_BRANCH26 relocation to another symbol + a non-zero offset could be emitted but the linkers don't support this type of relocation and cause incorrect relocations and crashes. Avoid emitting this type of relocations.

[compnerd]

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

[hjyamauchi]

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Yeah. To be clearer, what I observed was that the relocation type works as long as the offset that's embedded in the instruction is zero (I suspect that the linker doesn't use the offset in relocation or assumes it's zero.) I also tried a small local patch in lld to take this non-zero offset into account in the relocation and it also avoided the crashes I'm seeing.

[mstorsjo]

In D155732#4515825, @hjyamauchi wrote:

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Yeah. To be clearer, what I observed was that the relocation type works as long as the offset that's embedded in the instruction is zero (I suspect that the linker doesn't use the offset in relocation or assumes it's zero.) I also tried a small local patch in lld to take this non-zero offset into account in the relocation and it also avoided the crashes I'm seeing.

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.

[efriedma]

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

The description sounds like it's also not supported by link.exe? Not sure.

---

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

There's a bunch of code for A.isTemporary() a few lines earlier; would it make sense to integrate with that? This is sort of similar to what we do with OffsetSymbols, except the maximum offset is zero.

You can't create symbols at arbitrary offsets from the start of a section; you need to check that the symbol actually points somewhere within the section.

llvm/lib/MC/WinCOFFObjectWriter.cpp

963

We shouldn't end up in recordRelocation for branches where the source and destination are in the same section, I think.

[hjyamauchi]

In D155732#4516410, @efriedma wrote:

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

The description sounds like it's also not supported by link.exe? Not sure.

Yes. As far as I can tell, link.exe doesn't support this relocation type with an offset and that matches what lld does.

[compnerd]

In D155732#4516320, @mstorsjo wrote:

In D155732#4515825, @hjyamauchi wrote:

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Yeah. To be clearer, what I observed was that the relocation type works as long as the offset that's embedded in the instruction is zero (I suspect that the linker doesn't use the offset in relocation or assumes it's zero.) I also tried a small local patch in lld to take this non-zero offset into account in the relocation and it also avoided the crashes I'm seeing.

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

I would agree, but it seems that link also does not support the relocation with addend, and I would rather that we retain compatibility with the reference implementation. It sounds like it is only the addend form that we would need to avoid on Windows, not the general relocation.

I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.

[MaskRay]

llvm/lib/MC/WinCOFFObjectWriter.cpp

963

Ideally we should reuse the temporary symbol, not creating a new one.

The ELF approach is to omit temporary symbols only when unused by relocations (see ELFWriter::isInSymtab).

COFF D14975 may give some insights as well.

[hjyamauchi]

In D155732#4519183, @compnerd wrote:

In D155732#4516320, @mstorsjo wrote:

In D155732#4515825, @hjyamauchi wrote:

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Yeah. To be clearer, what I observed was that the relocation type works as long as the offset that's embedded in the instruction is zero (I suspect that the linker doesn't use the offset in relocation or assumes it's zero.) I also tried a small local patch in lld to take this non-zero offset into account in the relocation and it also avoided the crashes I'm seeing.

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

I would agree, but it seems that link also does not support the relocation with addend, and I would rather that we retain compatibility with the reference implementation. It sounds like it is only the addend form that we would need to avoid on Windows, not the general relocation.

I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.

branch26-test-log.txt7 KBDownload

The attach text file contains a test (*) and the command line trace that shows the relocation behaviors (without this patch) in the first half, and the same sequence of commands with this patch in the second half.

The first part shows that both link.exe and lld relocate the branches to the beginning of the text and the other section, as opposed to the ret instructions, respectively, because the offsets embedded in the instruction are ignored. The second part shows that, with this patch, the linkers relocate them as expected.

(*) A tweaked version of the test in this patch (because some important details were unexpectedly lost along the test reduction and nops were added to avoid accidental offset correctness as small offsets from a symbol can happen to be correct).

[hjyamauchi]

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

If you mean like the following, that would unintentionally trigger when we drop the relocation in the newly-added code (in the if (MCSec == &A.getSection()) case) because IsResolved is false.

case AArch64::fixup_aarch64_pcrel_branch26:
case AArch64::fixup_aarch64_pcrel_call26:
  if (TheTriple.isOSBinFormatCOFF() && !IsResolved && SignedValue != 0) {
    // MSVC link.exe and lld do not support this relocation type with a non-zero offset
    Ctx.reportError(Fixup.getLoc(), "fixup value not supported by the linkers");
  }

[hjyamauchi]

There's a bunch of code for A.isTemporary() a few lines earlier; would it make sense to integrate with that? This is sort of similar to what we do with OffsetSymbols, except the maximum offset is zero.

That may make sense as long as it's okay to check if the relocation type is fixup_aarch64_pcrel_branch26 or fixup_aarch64_pcrel_call26 before the getRelocType() call.

[hjyamauchi]

You can't create symbols at arbitrary offsets from the start of a section; you need to check that the symbol actually points somewhere within the section.

Can you clarify? Does it mean that we need to check A.isInSection() && Layout.getSymbolOffset(A) < getSectionAddressSize(&A.getSection()) before we create a new symbol at where symbol A is? What should happen if that's not the case?

[mstorsjo]

In D155732#4520749, @hjyamauchi wrote:

I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.

branch26-test-log.txt7 KBDownload

The attach text file contains a test (*) and the command line trace that shows the relocation behaviors (without this patch) in the first half, and the same sequence of commands with this patch in the second half.

The first part shows that both link.exe and lld relocate the branches to the beginning of the text and the other section, as opposed to the ret instructions, respectively, because the offsets embedded in the instruction are ignored. The second part shows that, with this patch, the linkers relocate them as expected.

(*) A tweaked version of the test in this patch (because some important details were unexpectedly lost along the test reduction and nops were added to avoid accidental offset correctness as small offsets from a symbol can happen to be correct).

Thanks for the test transcript! It does indeed look like link.exe ignores the existing offset. I also independently tested this, and it actually looks like link.exe blindly ors the target offset bits on top of the existing opcode, just like lld does: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.6/lld/COFF/Chunks.cpp#L306-L322 Depending on the contents of the original offset and the intended symbol difference, the offset may end up being partially visible or not. In any case - the linker doesn't indeed seem to be able to handle immediate offsets here.

Strictly speaking I would consider that a bug in both linkers, but I totally agree we should avoid producing such relocations. I didn't have time to look at the implementation strategy itself yet (but trust others' judgement on it).

[compnerd]

Thanks for the test transcript! It does indeed look like link.exe ignores the existing offset. I also independently tested this, and it actually looks like link.exe blindly ors the target offset bits on top of the existing opcode, just like lld does: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.6/lld/COFF/Chunks.cpp#L306-L322 Depending on the contents of the original offset and the intended symbol difference, the offset may end up being partially visible or not. In any case - the linker doesn't indeed seem to be able to handle immediate offsets here.

My recollection is that for most relocations, link does not support (in GNU parlance) partial in-place relocations, that is, any addend is generally (though not always) ignored. It was a pretty big divergence between GNU and MSVC semantics.

Strictly speaking I would consider that a bug in both linkers, but I totally agree we should avoid producing such relocations. I didn't have time to look at the implementation strategy itself yet (but trust others' judgement on it).

I'm not sure I consider it a bug since that is the normal handling of relocations, but certainly a lacking useful feature. At least, it sounds like everyone is now in agreement on avoiding producing the relocation for Windows then.

[hjyamauchi]

In D155732#4522178, @compnerd wrote:

Thanks for the test transcript! It does indeed look like link.exe ignores the existing offset. I also independently tested this, and it actually looks like link.exe blindly ors the target offset bits on top of the existing opcode, just like lld does: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.6/lld/COFF/Chunks.cpp#L306-L322 Depending on the contents of the original offset and the intended symbol difference, the offset may end up being partially visible or not. In any case - the linker doesn't indeed seem to be able to handle immediate offsets here.

The lld code linked here is exactly what I locally modified to experiment and saw the crash go away with.

My recollection is that for most relocations, link does not support (in GNU parlance) partial in-place relocations, that is, any addend is generally (though not always) ignored. It was a pretty big divergence between GNU and MSVC semantics.

Strictly speaking I would consider that a bug in both linkers, but I totally agree we should avoid producing such relocations. I didn't have time to look at the implementation strategy itself yet (but trust others' judgement on it).

I'm not sure I consider it a bug since that is the normal handling of relocations, but certainly a lacking useful feature. At least, it sounds like everyone is now in agreement on avoiding producing the relocation for Windows then.

Yeah, as I implied by creating this revision, I also vote for avoiding this relocation type for compatibility.

[efriedma]

In D155732#4521021, @hjyamauchi wrote:

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

If you mean like the following, that would unintentionally trigger when we drop the relocation in the newly-added code (in the if (MCSec == &A.getSection()) case) because IsResolved is false.

case AArch64::fixup_aarch64_pcrel_branch26:
case AArch64::fixup_aarch64_pcrel_call26:
  if (TheTriple.isOSBinFormatCOFF() && !IsResolved && SignedValue != 0) {
    // MSVC link.exe and lld do not support this relocation type with a non-zero offset
    Ctx.reportError(Fixup.getLoc(), "fixup value not supported by the linkers");
  }

I still don't understand why the MCSec == &A.getSection() case is relevant; if the offset is constant, it should be resolved, since "resolved" means "doesn't need a relocation".

In D155732#4521029, @hjyamauchi wrote:

You can't create symbols at arbitrary offsets from the start of a section; you need to check that the symbol actually points somewhere within the section.

Can you clarify? Does it mean that we need to check A.isInSection() && Layout.getSymbolOffset(A) < getSectionAddressSize(&A.getSection()) before we create a new symbol at where symbol A is? What should happen if that's not the case?

Something like that, yes. If it fails, we need to print an error, because we don't have any way to encode it. At least, I don't think we have a way to encode it? Not sure off the top of my head what offsets the linker would tolerate.

[hjyamauchi]

Updated the revision with the better test and changes per review comments.

[hjyamauchi]

In D155732#4521023, @hjyamauchi wrote:

There's a bunch of code for A.isTemporary() a few lines earlier; would it make sense to integrate with that? This is sort of similar to what we do with OffsetSymbols, except the maximum offset is zero.

That may make sense as long as it's okay to check if the relocation type is fixup_aarch64_pcrel_branch26 or fixup_aarch64_pcrel_call26 before the getRelocType() call.

Done.

[hjyamauchi]

llvm/lib/MC/WinCOFFObjectWriter.cpp

963

Done. Reused the temporary symbol.

963

(Please see the other comment discussion)

[hjyamauchi]

In D155732#4523640, @efriedma wrote:

In D155732#4521021, @hjyamauchi wrote:

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

If you mean like the following, that would unintentionally trigger when we drop the relocation in the newly-added code (in the if (MCSec == &A.getSection()) case) because IsResolved is false.

case AArch64::fixup_aarch64_pcrel_branch26:
case AArch64::fixup_aarch64_pcrel_call26:
  if (TheTriple.isOSBinFormatCOFF() && !IsResolved && SignedValue != 0) {
    // MSVC link.exe and lld do not support this relocation type with a non-zero offset
    Ctx.reportError(Fixup.getLoc(), "fixup value not supported by the linkers");
  }

I still don't understand why the MCSec == &A.getSection() case is relevant; if the offset is constant, it should be resolved, since "resolved" means "doesn't need a relocation".

Good point. Removed the MCSec == &A.getSection() case. There seem to be edges cases where relocations are kept even if the offset is constant such as a) relocations between functions are kept for the MSVC linker features as in WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl and b) the ADRP instruction / fixup_aarch64_pcrel_adrp_imm21 type as in AArch64AsmBackend::shouldForceRelocation. The crash case and the test we have here is due to a). So we cannot drop relocations here. Added the suggested check in adjustFixupValue.

[hjyamauchi]

Can you clarify? Does it mean that we need to check A.isInSection() && Layout.getSymbolOffset(A) < getSectionAddressSize(&A.getSection()) before we create a new symbol at where symbol A is? What should happen if that's not the case?

Something like that, yes. If it fails, we need to print an error, because we don't have any way to encode it. At least, I don't think we have a way to encode it? Not sure off the top of my head what offsets the linker would tolerate.

Done.

[hjyamauchi]

Please take another look.

[efriedma]

llvm/lib/MC/WinCOFFObjectWriter.cpp

953

Asm.getContext().reportError(), like code earlier in this function.

962

I don't understand why it's safe to throw away the "FixedValue" here; there could be an explicit offset.

[hjyamauchi]

Update to address comments.

[hjyamauchi]

Please take another look. @efriedma

llvm/lib/MC/WinCOFFObjectWriter.cpp

953

Done.

962

Done.

[compnerd]

Thinking about this a bit further, I think that it might be better to conform to the Microsoft behaviour here. If we look at what MASM does, there is no concept of an "assembler temporary" hidden symbols. We should instead always emit all symbols into the symbol table. The linker is responsible for filtering the symbols that are emitted into the symbol table (only symbols marked for DLL export should be emitted there). This is one of the fundamental differences between PE and COFF. COFF will record all symbols (e.g. consider what happens when you build with debug information, all the temporary symbols for lines, etc. are emitted into the object file), PE will emit only the public symbols (technically, it does not have a symbol table at all, only the content needed for the EAT). This would bring us more in line with the expectations of the platform and the object file format and also avoid the need for any special casing here - we would form the relocation as per usual, the linker will have the necessary context, and then we would simply drop the "temporary" symbol which is not marked for DLL Export.

[efriedma]

Thinking about this a bit further, I think that it might be better to conform to the Microsoft behaviour here. If we look at what MASM does, there is no concept of an "assembler temporary" hidden symbols. We should instead always emit all symbols into the symbol table.

I don't think that we can directly compare what we do to masm; we support an assembler dialect which is not masm, and that includes branching where the destination is an offset from a symbol. Given that, I think we need this logic in WinCOFFObjectWriter, whether or not we emit all temporary symbols. (The change in AArch64AsmBackend.cpp is necessary either way.)

Also, emitting all temporary symbols significantly bloats object files. (I don't have numbers at hand, but I recall it being significant.)

llvm/lib/MC/WinCOFFObjectWriter.cpp

962

This still looks wrong in the case where SymbolMap[&A] is non-null; even if the initial symbol lookup succeeds, we still need to preserve the offset.

[compnerd]

I don't think that we can directly compare what we do to masm; we support an assembler dialect which is not masm, and that includes branching where the destination is an offset from a symbol. Given that, I think we need this logic in WinCOFFObjectWriter, whether or not we emit all temporary symbols. (The change in AArch64AsmBackend.cpp is necessary either way.)

I guess the piece that I'm wondering is why we would need that if the local symbol is available because the label is preserved, the linker should be able to resolve the symbol relative offset and then discard the temporary symbol I thought. What condition am I overlooking?

Also, emitting all temporary symbols significantly bloats object files. (I don't have numbers at hand, but I recall it being significant.)

Oh, I completely agree with you on that point. I wouldn't be surprised at all at it being significant. I think that matching semantics is something we should do any way.

[efriedma]

In D155732#4570643, @compnerd wrote:

I don't think that we can directly compare what we do to masm; we support an assembler dialect which is not masm, and that includes branching where the destination is an offset from a symbol. Given that, I think we need this logic in WinCOFFObjectWriter, whether or not we emit all temporary symbols. (The change in AArch64AsmBackend.cpp is necessary either way.)

I guess the piece that I'm wondering is why we would need that if the local symbol is available because the label is preserved, the linker should be able to resolve the symbol relative offset and then discard the temporary symbol I thought. What condition am I overlooking?

I was thinking of the case of branching to "x+4", or something like that, where there isn't any existing symbol at the offset in question. We could forbid that, I guess, but it seems like that just shifts complexity elsewhere.

[compnerd]

In D155732#4570674, @efriedma wrote:

In D155732#4570643, @compnerd wrote:

I don't think that we can directly compare what we do to masm; we support an assembler dialect which is not masm, and that includes branching where the destination is an offset from a symbol. Given that, I think we need this logic in WinCOFFObjectWriter, whether or not we emit all temporary symbols. (The change in AArch64AsmBackend.cpp is necessary either way.)

I guess the piece that I'm wondering is why we would need that if the local symbol is available because the label is preserved, the linker should be able to resolve the symbol relative offset and then discard the temporary symbol I thought. What condition am I overlooking?

I was thinking of the case of branching to "x+4", or something like that, where there isn't any existing symbol at the offset in question. We could forbid that, I guess, but it seems like that just shifts complexity elsewhere.

Right, I was thinking that we would have a(n extra) label at that point so the offset is always 0. We would generate a synthetic symbol for any case where the user is writing out an offset from a symbol.

e.g.

  .def sym
    .scl 2
    .type 32
  .endef
sym:
  nop
  nop
$LN$reloc:
  .long $LN$reloc
  ret

would be equivalent to:

  .def sym
    .scl 2
    .type 32
  .endef
sym:
  nop
  nop
  .long sym + 4
  ret

Edit: oh, I see what you mean - we do not know what the target was lowered as and we might be external to the symbol, which means that we cannot guarantee the label at that point ...

[rnk]

I agree that we can't emit all labels with the .L prefix into the object file symbol table. That would be prohibitively expensive.

We can, however, probably treat all private linkage symbols as internal if we don't already, so we get symbols for them in the symbol table, if this issue is still reachable from the IR level. We can probably also "upgrade" private symbols that the assembler reads to add them to the symbol table if they are used with this kind of fixup. Either way, I think we need the error handling added in the existing patch.

llvm/lib/MC/WinCOFFObjectWriter.cpp

953

Please add a test exercising this error case as well.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

320

Please add a test exercising the error case, presumably using something like the b .Lsym + 4 pattern.

Also, I think you can make the diagnostic message clearer. Something like, "cannot perform a PC-relative fixup with a non-zero symbol offset" (emphasize that the offset from a symbol must be zero).

[hjyamauchi]

Updated to treat all private linkage symbols as internal and add them to the symbol table, as Reid suggested. This fixes the issue with private linkage and the swift issue.

This also fixes the symbol class for private linkage for Windows/AArch64 from external to static to be in align with X86, and adds a test exercising the error case for the check in AArch64AsmBackend.

Please take another look.

[compnerd]

I think that I generally prefer this approach. I would however like to see a test for the storage class changes (and ideally split from this change - it fixes a problem needed to fix here, but is a completely standalone improvement).

[MaskRay]

llvm/test/MC/AArch64/coff-relocations-branch26-bad.s

1 ↗

(On Diff #550086)

In many occasions placing negative tests in the same file as the positive tests improves readability. You can use the --defsym ERR=1 pattern

18 ↗

(On Diff #550086)

New tests are recommended to add line/column information to diagnostics, similar to test/MC/ELF/cfi-scope-errors.s

[hjyamauchi]

In D155732#4588492, @compnerd wrote:

I think that I generally prefer this approach. I would however like to see a test for the storage class changes (and ideally split from this change - it fixes a problem needed to fix here, but is a completely standalone improvement).

Split the storage class change into https://reviews.llvm.org/D158122

[rnk]

lgtm, thanks!

[hjyamauchi]

Rebased over https://reviews.llvm.org/D158122. Addressed the comments on the test code.

llvm/test/MC/AArch64/coff-relocations-branch26-bad.s

1 ↗

(On Diff #550086)

Done.

18 ↗

(On Diff #550086)

Done.

llvm/lib/MC/WinCOFFObjectWriter.cpp

for (const auto &Section : Asm) {
     if ((Mode == NonDwoOnly && isDwoSection(Section)) ||
         (Mode == DwoOnly && !isDwoSection(Section)))
       continue;
     defineSection(static_cast<const MCSectionCOFF &>(Section), Layout);
   }
 
   if (Mode != DwoOnly)
     for (const MCSymbol &Symbol : Asm.symbols())
-      if (!Symbol.isTemporary())
+      // Define non-temporary or temporary static (private-linkage) symbols
+      if (!Symbol.isTemporary() ||
+          cast<MCSymbolCOFF>(Symbol).getClass() == COFF::IMAGE_SYM_CLASS_STATIC)
         DefineSymbol(Symbol, Asm, Layout);
 }
 
 void WinCOFFWriter::recordRelocation(MCAssembler &Asm,
                                      const MCAsmLayout &Layout,
                                      const MCFragment *Fragment,
                                      const MCFixup &Fixup, MCValue Target,
                                      uint64_t &FixedValue) {
void WinCOFFWriter::recordRelocation(MCAssembler &Asm,
   }
 
   COFFRelocation Reloc;
 
   Reloc.Data.SymbolTableIndex = 0;
   Reloc.Data.VirtualAddress = Layout.getFragmentOffset(Fragment);
 
   // Turn relocations for temporary symbols into section relocations.
-  if (A.isTemporary()) {
+  if (A.isTemporary() && !SymbolMap[&A]) {
     MCSection *TargetSection = &A.getSection();
     assert(
         SectionMap.contains(TargetSection) &&
         "Section must already have been defined in executePostLayoutBinding!");
     COFFSection *Section = SectionMap[TargetSection];
     Reloc.Symb = Section->Symbol;
     FixedValue += Layout.getSymbolOffset(A);
     // Technically, we should do the final adjustments of FixedValue (below)
void WinCOFFWriter::recordRelocation(MCAssembler &Asm,
   Reloc.Data.VirtualAddress += Fixup.getOffset();
   Reloc.Data.Type = OWriter.TargetObjectWriter->getRelocType(
       Asm.getContext(), Target, Fixup, SymB, Asm.getBackend());
 
   // The *_REL32 relocations are relative to the end of the relocation,
   // not to the start.
   if ((Header.Machine == COFF::IMAGE_FILE_MACHINE_AMD64 &&
        Reloc.Data.Type == COFF::IMAGE_REL_AMD64_REL32) ||
       (Header.Machine == COFF::IMAGE_FILE_MACHINE_I386 &&
        Reloc.Data.Type == COFF::IMAGE_REL_I386_REL32) ||
       (Header.Machine == COFF::IMAGE_FILE_MACHINE_ARMNT &&
        Reloc.Data.Type == COFF::IMAGE_REL_ARM_REL32) ||
       (Header.Machine == COFF::IMAGE_FILE_MACHINE_ARM64 &&
        Reloc.Data.Type == COFF::IMAGE_REL_ARM64_REL32))
     FixedValue += 4;
 
   if (Header.Machine == COFF::IMAGE_FILE_MACHINE_ARMNT) {
     switch (Reloc.Data.Type) {
     case COFF::IMAGE_REL_ARM_ABSOLUTE:
     case COFF::IMAGE_REL_ARM_ADDR32:
     case COFF::IMAGE_REL_ARM_ADDR32NB:
     case COFF::IMAGE_REL_ARM_TOKEN:
     case COFF::IMAGE_REL_ARM_SECTION:
     case COFF::IMAGE_REL_ARM_SECREL:
       break;
     case COFF::IMAGE_REL_ARM_BRANCH11:
     case COFF::IMAGE_REL_ARM_BLX11:

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

case AArch64::fixup_aarch64_pcrel_branch14:
     if (!isInt<16>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
     if (Value & 0x3)
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     return (Value >> 2) & 0x3fff;
   case AArch64::fixup_aarch64_pcrel_branch26:
   case AArch64::fixup_aarch64_pcrel_call26:
+    if (TheTriple.isOSBinFormatCOFF() && !IsResolved && SignedValue != 0) {
+      // MSVC link.exe and lld do not support this relocation type
+      // with a non-zero offset
+      Ctx.reportError(Fixup.getLoc(),
+                      "cannot perform a PC-relative fixup with a non-zero "
+                      "symbol offset");
+    }
     // Signed 28-bit immediate
     if (!isInt<28>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
     if (Value & 0x3)
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     return (Value >> 2) & 0x3ffffff;
   case FK_Data_1:

llvm/test/MC/AArch64/coff-relocations-branch26.s

+// RUN: llvm-mc -triple aarch64-unknown-windows-msvc -filetype obj %s -o - | llvm-objdump -D -r - | FileCheck %s
+// RUN: not llvm-mc -triple aarch64-unknown-windows-msvc -filetype obj --defsym ERR=1 %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
+
+    .text
+main:
+    nop
+    b .Ltarget
+    b .Lother_target
+
+// A privte label target in the same section
+    .def .Ltarget
+    .scl 3
+    .type 32
+    .endef
+    .p2align 2
+.Ltarget:
+    ret
+
+// A privte label target in another section
+    .section "other"
+    nop
+    nop
+    nop
+    nop
+    nop
+    nop
+    nop
+    nop
+    .def .Lother_target
+    .scl 3
+    .type 32
+    .endef
+    .p2align 2
+.Lother_target:
+    ret
+
+// Check that both branches have a relocation with a zero offset.
+//
+// CHECK: 0000000000000000 <main>:
+// CHECK:        0: d503201f      nop
+// CHECK:        4: 14000000      b       0x4 <main+0x4>
+// CHECK:                 0000000000000004:  IMAGE_REL_ARM64_BRANCH26     .Ltarget
+// CHECK:        8: 14000000      b       0x8 <main+0x8>
+// CHECK:                 0000000000000008:  IMAGE_REL_ARM64_BRANCH26     .Lother_target
+// CHECK: 000000000000000c <.Ltarget>:
+// CHECK:        c: d65f03c0      ret
+// CHECK: 0000000000000000 <other>:
+// CHECK:        0: d503201f      nop
+// CHECK:        4: d503201f      nop
+// CHECK:        8: d503201f      nop
+// CHECK:        c: d503201f      nop
+// CHECK:       10: d503201f      nop
+// CHECK:       14: d503201f      nop
+// CHECK:       18: d503201f      nop
+// CHECK:       1c: d503201f      nop
+// CHECK: 0000000000000020 <.Lother_target>:
+// CHECK:       20: d65f03c0      ret
+
+.ifdef ERR
+    .section "err"
+err:
+    nop
+    b .Lerr_target+4
+// ERR: [[#@LINE-1]]:5: error: cannot perform a PC-relative fixup with a non-zero symbol offset
+
+    .def .Lerr_target
+    .scl 3
+    .type 32
+    .p2align 2
+    .endef
+.Lerr_target:
+    nop
+    nop
+    ret
+.endif