[PATCH] D155732: [MC][COFF][AArch64] Avoid incorrect IMAGE_REL_ARM64_BRANCH26 relocations.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

hjyamauchi marked an inline comment as done.
hjyamauchi added a comment.

In D155732#4523640 <https://reviews.llvm.org/D155732#4523640>, @efriedma wrote:

> In D155732#4521021 <https://reviews.llvm.org/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`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155732/new/

https://reviews.llvm.org/D155732