This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add warn flags to enable/disable warnings on -install_name
ClosedPublic

Authored by thevinster on Nov 9 2021, 6:12 PM.

Details

Summary

ld64 doesn't warn on builds using -install_name if it's a bundle. But, the
current warning is nice to have because install_name only works with dylib.
To prevent an overflow of warnings in build logs and have parity with ld64,
create a --warn-dylib-install-name and --warn-no-dylib-install-name flag
that enables this LLD specific warning.

Diff Detail

Event Timeline

thevinster created this revision.Nov 9 2021, 6:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
thevinster edited the summary of this revision. (Show Details)Nov 9 2021, 6:16 PM
thevinster published this revision for review.Nov 9 2021, 6:21 PM
thevinster edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2021, 6:22 PM
thakis added a subscriber: thakis.Nov 9 2021, 6:41 PM

The flag doesn't have an effect on bundles though. The warning is correct. See D98259 – ld doesn't have this warning at all.

In Chromium, this warning did identify a bunch of places where we passed this flag where it didn't have an effect, so having the warning seems vaguely useful to me.

Maybe we should instead have a flag to disable the warning?

Maybe we should instead have a flag to disable the warning?

The reason for this change is that even though it's a no-op, it's polluting quite bunch of internal build logs which make it hard to uncover what warnings are actually problems and what aren't. As far as the flag to disable the warning, would it better to mirror ld64's behavior and have a flag to enable this check instead? (Alternatively, we could also make the flag to disable the warning a default as well). wdyt?

int3 added a subscriber: int3.Nov 10 2021, 7:52 AM

Yeah, we shouldn't change the error message, since the old one was right :)

I do think we should mirror ld64's (lack of) warnings by default though, since it makes it a better drop-in replacement. I think gating these additional warnings behind something like a --warn-strict flag would be good.

Introduce --warn-strict flag to enable custom warnings for LLD

thevinster retitled this revision from [lld-macho] Do not warn on install_name for bundles to [lld-macho] Introduce LLD custom warnings with --warn-strict.Nov 10 2021, 4:45 PM
thevinster edited the summary of this revision. (Show Details)
int3 added a comment.Nov 10 2021, 4:51 PM

lgtm, but @thakis should have a look too

ld64 has a bunch of -no_warn_foo flags to disable specific warnings. That's also the model that clang and lld/COFF use. So maybe we should have a dedicated flag for this warning instead of some umbrella thing?

thevinster edited the summary of this revision. (Show Details)

Use dedicated warning instead of umbrella

thevinster retitled this revision from [lld-macho] Introduce LLD custom warnings with --warn-strict to [lld-macho] Avoid warning on -install-name if -dylib is not provided.Nov 15 2021, 12:25 PM
thevinster edited the summary of this revision. (Show Details)
int3 added a comment.Nov 15 2021, 3:30 PM

I'm a bit hesitant about using that scheme because it seems like we could end up with dozens of flags, each for a different warning. But I suppose if the other ports are doing it already then it's nice to follow them

I'm a bit hesitant about using that scheme because it seems like we could end up with dozens of flags, each for a different warning. But I suppose if the other ports are doing it already then it's nice to follow them

Isn't that _good_ though? Clang has hundreds of warning flags, and I think that's generally appreciated. We should still pick good defaults of course, but likely different projects will want to care about different things.

The patch generally looks good, but I'd probably also add a --warn-dylib-install-name to override the no form, name the config variable to use the positive name ("don't use no double negation") and mention "[--warn-dylib-install-name]" in the warning text so that users know the warning is toggleable (like clang does).

but I'd probably also add a --warn-dylib-install-name to override the no form

Just so I understand here, you're suggesting to rename the current config to --warn-dylib-install-name? It doesn't make sense to have a --warn-no and a --warn because by default, the warning isn't enabled. The reason why I had the name --warn-no-dylib-install-name is to make it clear to readers that install-name can't used with other flags other than -dylib (hence the no...). I do think the name can be a bit confusing with the --warn-no structure, but the name, --warn-dylib-install-name, reads it as if the code should warn if install-name is used with -dylib. Perhaps, --warn-install-name-no-dylib?

but I'd probably also add a --warn-dylib-install-name to override the no form

Just so I understand here, you're suggesting to rename the current config to --warn-dylib-install-name?

No, I mean also adding a --warn-dylib-install-name

It doesn't make sense to have a --warn-no and a --warn because by default, the warning isn't enabled.

Many many flags have both positive and negative forms, in many tools, despite one of them being the default. This is useful because:

a) If you have some hairball build where something passes the non-default flag explicitly for all targets but you want to undo it locally for some target, you can pass the other variant further on
b) It makes it easy to show the default in --help output (just put " (default)" at the end of the help text that has the default. bin/lld-link '/?' has many examples of this.

The reason why I had the name --warn-no-dylib-install-name is to make it clear to readers that install-name can't used with other flags other than -dylib (hence the no...). I do think the name can be a bit confusing with the --warn-no structure, but the name, --warn-dylib-install-name, reads it as if the code should warn if install-name is used with -dylib. Perhaps, --warn-install-name-no-dylib?

Include both pos and neg form of the warning, default to neg

thevinster edited the summary of this revision. (Show Details)Nov 16 2021, 11:58 PM
int3 added a comment.Nov 17 2021, 12:52 AM

Isn't that _good_ though? Clang has hundreds of warning flags, and I think that's generally appreciated. We should still pick good defaults of course, but likely different projects will want to care about different things.

What I had in mind as the alternative was something like --no-warn <warning name>. Sorry, I realize now that I totally neglected to add that context 😅

lld/MachO/Options.td
76

I think no-warn makes a lot more sense than warn-no here

(btw, LLD-ELF's Options.td has a macro thing to define both the "yes" and "no" versions of a flag. We can consider that as we add more of these.

lld/test/MachO/install-name.s
20

either remove the redirect or check that the output lacks a warning :)

thakis accepted this revision.Nov 17 2021, 10:30 AM

This lg, thanks. (Either as-is or with s/warn-no/no-warn/)

Please make sure the first line of the commit isn't "[lld-macho] Avoid warning on -install-name if -dylib is not provided" but something updated when committing. ("[lld-macho] Add flag to disable warning on pointless -install_name" or similar)

lld/MachO/Options.td
76

--warn-no kind of matches clang's -Wno, which seems vaguely nice to me. But no strong preference.

no-warn kind of matches ld64's various -warn_no_ flags, which also seems vaguely nice :)

This revision is now accepted and ready to land.Nov 17 2021, 10:30 AM
thevinster marked 2 inline comments as done.Nov 17 2021, 11:04 AM
thevinster added inline comments.
lld/MachO/Options.td
76

I'll switch to no-warn as an effort to be more coupled with ld64's variation.

lld/test/MachO/install-name.s
20

I think the combination of %lld and the fact that the command didn't fail is enough signal to know that the output lacks a warning.

thevinster retitled this revision from [lld-macho] Avoid warning on -install-name if -dylib is not provided to [lld-macho] Add warn flags to enable/disable warnings on -install_name.Nov 17 2021, 3:13 PM
thevinster marked an inline comment as done.

s/warn-no/no-warn