This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Silently ignore the -objc_abi_version
ClosedPublic

Authored by keith on Nov 2 2021, 4:46 PM.

Details

Reviewers
gkm
int3
Group Reviewers
Restricted Project
Commits
rGe7fdff403e84: [lld-macho] Silently ignore the -objc_abi_version
Summary

This undocumented ld64 flag, based on the most recent ld64 source dump
from Xcode 12, only applies to i386. It seems like on all newer
architectures this behavior is the default.

Diff Detail

Event Timeline

keith created this revision.Nov 2 2021, 4:46 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
keith requested review of this revision.Nov 2 2021, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 4:46 PM
keith added a comment.Nov 2 2021, 4:47 PM

This one is a bit of a stretch, I'm not sure if you want to start dropping options like this, especially since Apple reserves the right to change the behavior over time. The only benefit in ignoring this is without dropping it, if you want to maintain compatibility and users are passing this, they cannot eliminate the warning produced (or the failure if they're using -fatal_warnings)

oontvoo added a subscriber: oontvoo.Nov 2 2021, 6:23 PM
oontvoo added inline comments.
lld/MachO/Options.td
1324

Why do we need this? Doesn't removing the Flags<HelpHidden> effectively ignore it silently?

(should also add a test to silent-ignore.test)

keith added inline comments.Nov 2 2021, 6:32 PM
lld/MachO/Options.td
1324

This is primarily for the C++ change that stops this category from printing a warning. That's what silently means in this case.

int3 added a subscriber: int3.Nov 2 2021, 7:00 PM

I like this, but let's see if anyone else chimes in over the next few days. I agree that we should add it to silent-ignore.test though.

keith added a comment.Nov 2 2021, 9:14 PM

I like this, but let's see if anyone else chimes in over the next few days. I agree that we should add it to silent-ignore.test though.

I just took a list at this test, it seems like it's a bit invalid today. It looks like --version short circuits too early and the invalid flags aren't a problem in that case. -objc_abi_version is actually already in there. Am I reading the recommendation right there? I can update it to actually try to link a file so it gets far enough to error if that's the intent?

int3 added a comment.Nov 3 2021, 8:20 AM

Oh, I remember what that test was checking for now... it was just there to make sure that any unimplemented flags were still being parsed correctly, regardless of whether we were emitting diagnostic warnings/errors for them. (Our Options.td initially had a bunch of flags expecting wrong numbers of arguments, which messed up the parsing of all the other flags even when they were ignored. That's the error condition that the --not-an-ignored-argument run command is checking for.)

I think for this diff, we can add another RUN command to the silent-ignore.test, but this time specifically checking that we don't emit any diagnostics for -objc_abi_version.

keith updated this revision to Diff 384555.Nov 3 2021, 12:19 PM
keith marked an inline comment as done.

Add test making sure flag doesn't warn

int3 accepted this revision.Nov 3 2021, 9:40 PM

some additional comments would be good, but otherwise lgtm. Thanks!

lld/test/MachO/silent-ignore.s
4
19
This revision is now accepted and ready to land.Nov 3 2021, 9:40 PM
keith updated this revision to Diff 384655.Nov 3 2021, 9:49 PM
keith marked 2 inline comments as done.

Add comments to test

This revision was automatically updated to reflect the committed changes.
lld/test/MachO/silent-ignore.s