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

[thevinster]

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.

[thakis]

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?

[thevinster]

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]

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.

[thevinster]

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

[int3]

lgtm, but @thakis should have a look too

[thakis]

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]

Use dedicated warning instead of umbrella

[int3]

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

[thakis]

In D113534#3133230, @int3 wrote:

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.

[thakis]

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).

[thevinster]

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?

[thakis]

In D113534#3135231, @thevinster wrote:

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?

[thevinster]

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

[int3]

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]

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 :)

[thevinster]

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]

s/warn-no/no-warn

lld/MachO/Config.h

struct Configuration {
   bool emitFunctionStarts = false;
   bool emitBitcodeBundle = false;
   bool emitDataInCodeInfo = false;
   bool emitEncryptionInfo = false;
   bool timeTraceEnabled = false;
   bool dataConst = false;
   bool dedupLiterals = true;
   bool omitDebugInfo = false;
+  bool warnNoDylibInstallName = false;
   uint32_t headerPad;
   uint32_t dylibCompatibilityVersion = 0;
   uint32_t dylibCurrentVersion = 0;
   uint32_t timeTraceGranularity = 500;
   unsigned optimize;
   std::string progName;
 
   // For `clang -arch arm64 -arch x86_64`, clang will:

lld/MachO/Driver.cpp

bool macho::link(ArrayRef<const char *> argsArr, bool canExitEarly,
   config->emitFunctionStarts =
       args.hasFlag(OPT_function_starts, OPT_no_function_starts, true);
   config->emitBitcodeBundle = args.hasArg(OPT_bitcode_bundle);
   config->emitDataInCodeInfo =
       args.hasFlag(OPT_data_in_code_info, OPT_no_data_in_code_info, true);
   config->icfLevel = getICFLevel(args);
   config->dedupLiterals = args.hasArg(OPT_deduplicate_literals) ||
                           config->icfLevel != ICFLevel::none;
+  config->warnNoDylibInstallName = args.hasArg(OPT_warn_no_dylib_install_name);
 
   // FIXME: Add a commandline flag for this too.
   config->zeroModTime = getenv("ZERO_AR_DATE");
 
   std::array<PlatformKind, 3> encryptablePlatforms{
       PlatformKind::iOS, PlatformKind::watchOS, PlatformKind::tvOS};
   config->emitEncryptionInfo =
       args.hasFlag(OPT_encryptable, OPT_no_encryption,
                    is_contained(encryptablePlatforms, config->platform()));
 
 #ifndef LLVM_HAVE_LIBXAR
   if (config->emitBitcodeBundle)
     error("-bitcode_bundle unsupported because LLD wasn't built with libxar");
 #endif
 
   if (const Arg *arg = args.getLastArg(OPT_install_name)) {
-    if (config->outputType != MH_DYLIB)
+    if (config->warnNoDylibInstallName && config->outputType != MH_DYLIB)
       warn(arg->getAsString(args) + ": ignored, only has effect with -dylib");
     else
       config->installName = arg->getValue();
   } else if (config->outputType == MH_DYLIB) {
     config->installName = config->finalOutput;
   }
 
   if (args.hasArg(OPT_mark_dead_strippable_dylib)) {

lld/MachO/Options.td

def lto_O: Joined<["--"], "lto-O">,
     HelpText<"Set optimization level for LTO (default: 2)">,
     MetaVarName<"<opt-level>">,
     Group<grp_lld>;
 def thinlto_cache_policy: Joined<["--"], "thinlto-cache-policy=">,
     HelpText<"Pruning policy for the ThinLTO cache">,
     Group<grp_lld>;
 def O : JoinedOrSeparate<["-"], "O">,
     HelpText<"Optimize output file size">;
+def warn_no_dylib_install_name: Joined<["--"], "warn-no-dylib-install-name">,
+    HelpText<"Warn on -install-name if -dylib is not passed">,
+    Group<grp_lld>;
 
 // This is a complete Options.td compiled from Apple's ld(1) manpage
 // dated 2018-03-07 and cross checked with ld64 source code in repo
 // https://github.com/apple-opensource/ld64 at git tag "512.4" dated
 // 2018-03-18.
 
 // Flags<[HelpHidden]> marks options that are not yet ported to lld,
 // and serve as a scoreboard for annotating our progress toward

lld/test/MachO/install-name.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t.o %s
 
-# RUN: %no_fatal_warnings_lld -o %t.exec %t.o -install_name foo 2>&1 \
-# RUN:     | FileCheck --check-prefix=WARN %s
+# RUN: %no_fatal_warnings_lld --warn-no-dylib-install-name -o %t.exec %t.o \
+# RUN:     -install_name foo 2>&1 | FileCheck --check-prefix=WARN %s
 # RUN: llvm-objdump --macho --all-headers %t.exec \
 # RUN:     | FileCheck --check-prefix=NO-ID %s
 
-# RUN: %no_fatal_warnings_lld -bundle -o %t.bundle %t.o -install_name foo 2>&1 \
-# RUN:     | FileCheck --check-prefix=WARN %s
+# RUN: %no_fatal_warnings_lld --warn-no-dylib-install-name -bundle -o %t.bundle %t.o \
+# RUN:     -install_name foo 2>&1 | FileCheck --check-prefix=WARN %s
+# RUN: llvm-objdump --macho --all-headers %t.bundle \
+# RUN:     | FileCheck --check-prefix=NO-ID %s
+
+# RUN: %lld -bundle -o %t.bundle %t.o -install_name foo 2>&1
 # RUN: llvm-objdump --macho --all-headers %t.bundle \
 # RUN:     | FileCheck --check-prefix=NO-ID %s
 
 # RUN: %lld -dylib -o %t.dylib %t.o -install_name foo 2>&1
 # RUN: llvm-objdump --macho --all-headers %t.dylib \
 # RUN:     | FileCheck --check-prefix=ID %s
 
 # WARN: warning: -install_name foo: ignored, only has effect with -dylib
 
 # NO-ID-NOT: LC_ID_DYLIB
 
 # ID:          cmd LC_ID_DYLIB
 # ID-NEXT: cmdsize
 # LID-NEXT:   name foo
 
 .globl _main
 _main:
   ret