This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] [Windows] Error out on unsupported symbol locations
ClosedPublic

Authored by mstorsjo on Aug 4 2020, 1:08 PM.

Details

Summary

These might occur in seemingly generic assembly. Previously when targeting COFF, they were silently ignored, which certainly won't give the right result. Instead clearly error out, to make it clear that the assembly needs to be adjusted for this target.

Also change a preexisting report_fatal_error into a proper error message, pointing out the offending source instruction. This isn't strictly an internal error, as it can be triggered by user input.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 4 2020, 1:08 PM
mstorsjo requested review of this revision.Aug 4 2020, 1:08 PM
efriedma added inline comments.Aug 4 2020, 1:51 PM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
119

VK_GOT isn't a bitmask.

If you need to mask, use the helpers AArch64MCExpr::getSymbolLoc() etc.

We probably also want to exclude other variants that don't make sense, like VK_GOTTPREL?

"unknown AArch64 symbol kind" isn't a great error message; can we explicitly say that we don't expect this kind of symbol on COFF targets?

mstorsjo added inline comments.Aug 5 2020, 2:45 AM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
119

VK_GOT isn't a bitmask.

If you need to mask, use the helpers AArch64MCExpr::getSymbolLoc() etc.

Ah, thanks, will do.

We probably also want to exclude other variants that don't make sense, like VK_GOTTPREL?

Yeah, that's probably safest. I initially went with just disallowing certain ones, but in the end it looks like only VK_ABS and VK_SECREL are supported at all, and this can be checked at the top outside of this switch, so instead of forbidding individual ones, just allow the few supported ones and error out on anything else.

"unknown AArch64 symbol kind" isn't a great error message; can we explicitly say that we don't expect this kind of symbol on COFF targets?

True, and we can also mention the explicit variant kind, for an even more understandable error message.

mstorsjo updated this revision to Diff 283174.Aug 5 2020, 2:48 AM
mstorsjo retitled this revision from [AArch64] [Windows] Error out on some ELF style GOT relative relocations to [AArch64] [Windows] Error out on unsupported symbol locations.
mstorsjo edited the summary of this revision. (Show Details)

Using getSymbolLoc(), moved the check to the top of the function, doing the check of symbol location regardless of relocation type (as only VK_ABS and VK_SECREL are supported), changed a preexisting report_fatal_error for unsupported relocation types into an error with a source location, as it can be triggered with custom assembly input.

I've managed to a rather large corpus of code (VLC, including around 100 third party library dependencies) with this patch, so it looks fairly safe that no other symbol locations than VK_ABS and VK_SECREL are being used in successfully built code so far.

compnerd accepted this revision.Aug 5 2020, 8:40 AM

This looks fantastic! I'm really liking the better diagnostics. The only thing I'd change is the "symbol kind %0 unsupported on COFF targets". I think that "relocation variant" is more precise than "symbol kind". Thanks for improving this.

This revision is now accepted and ready to land.Aug 5 2020, 8:40 AM
mstorsjo added a subscriber: hans.Aug 5 2020, 11:28 PM

@hans - WDYT about backporting this one? It's a little bit risky if the check would turn out to be too strict, but there's been no false positives in my test build corpus (which includes a fair bit of assembly source); the upside is for anybody working on porting (mainly assembly) code that isn't originally written for arm64/windows - as it previously wouldn't error out at build time on constructs that don't exist on windows and won't work as intended at runtime.

hans added a comment.Aug 6 2020, 4:48 AM

@hans - WDYT about backporting this one? It's a little bit risky if the check would turn out to be too strict, but there's been no false positives in my test build corpus (which includes a fair bit of assembly source); the upside is for anybody working on porting (mainly assembly) code that isn't originally written for arm64/windows - as it previously wouldn't error out at build time on constructs that don't exist on windows and won't work as intended at runtime.

I think the risk/reward ratio is decent here. Pushed as 3aec1c6a493f543d41caf99e5df9056776856941, please shout if you see any problems.