This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Fix crash with bitcode asm module

Authored by keith on Jan 30 2022, 1:39 PM.



When using llvm-libtool-darwin with LTO building llvm itself, it crashed
on a file with an asm module in the bitcode. This fixes that by
correctly registering the targets for this.

Diff Detail

Event Timeline

keith created this revision.Jan 30 2022, 1:39 PM
keith requested review of this revision.Jan 30 2022, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2022, 1:39 PM

Curious to hear if this is the right approach. Registering all of these targets seems like a big hammer but I think it's required unless we want to single out the targets darwin uses?

keith updated this revision to Diff 404412.Jan 30 2022, 3:24 PM


I'm afraid I don't have any particular insight into this area, so this needs someone else to review.

keith edited reviewers, added: smeenai, Roger, mehdi_amini; removed: jhenderson.Feb 3 2022, 9:03 AM
smeenai added a subscriber: int3.Feb 3 2022, 10:29 AM

I don't have direct experience with this either, but this seems sensible (I assume InitializeAllTargets is a pre-req for InitializeAllAsmParsers, although llvm-ar is just using InitializeAllTargetInfos). I'll let it sit for a few days to give others the chance to chime in though.

CC @int3, who set up LTO for LLD for Mach-O.

1 ↗(On Diff #404412)

I think you'll want REQUIRES lines for the targets you're using here (x86 and aarch64), to make the test not fail when LLVM is built without those targets. (I think just testing one architecture would be fine as well, to not require multiple targets to be enabled for running the test).

keith updated this revision to Diff 405735.Feb 3 2022, 11:59 AM

Update test REQUIRES, use llvm-ar logic instead

keith added a comment.Feb 3 2022, 12:00 PM

I've updated to just use the llvm-ar logic since I assume that's the most correct option.

1 ↗(On Diff #404412)

Ah yes thanks, I added both here. The reason I want both is because theoretically depending on how you register the targets, it may only work for your native target, and not any other targets for cross compilation, so having both here verifies that. If you think having both requires is too strict, I can split these into 2 test files?

Also note apparently in llvm/test the lit config is slightly different so the requires definitions are different here.

smeenai accepted this revision.Feb 3 2022, 1:17 PM

LGTM since we're just matching llvm-ar's logic.

1 ↗(On Diff #404412)

Yeah, that logic for testing both makes sense. I do think it'd be nice to split out into two files though (just to maximize testing coverage).

This revision is now accepted and ready to land.Feb 3 2022, 1:17 PM
keith updated this revision to Diff 405779.Feb 3 2022, 1:59 PM
keith marked 2 inline comments as done.

Split up test files

This revision was landed with ongoing or failed builds.Feb 4 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.