GCC compatibility: pass -z linker options to the linker

[bubbles231]

Summary

Typically linker options are protected with -Xlinker or -Wl, however certain sloppy Makefiles pass -z options directly to the compiler. This patch enables clang to recognize these options (because -z is not used by clang itself). Note the special case of Windows where the call path does not call AddLinkerOptions().

[• rafael]

adding cfe-commits

[joerg]

I'm against this. I haven't seen enough evidence of use to justify it, other options like -R are certainly a lot more popular...

[sylvestre.ledru]

I haven't seen the -R argument used. Do you have some pointers on this?
We have at least 6 cases in Debian of this usage (http://clang.debian.net/status.php?version=3.4.2&key=UNKNOWN_ARG)
To improve the compatibility with gcc, I think that make sense.

[joerg]

I find some of those build logs very telling. "Unroll loops", "inline functions" and "but don't try too hard". I really would think those should be flagged upstream as "please check the option first".

[joerg]

Sorry, wrong review. -R as alias for -rpath was the most common flag seen in the clang builds on NetBSD. Please keep in mind that GCC will effectively pass *all* unknown flags down to ld, which is why this sort of issue appeared in first place. That's also why I am quite strongly opposed against adding more non-driver flags in the front end.

[sylvestre.ledru]

In D4393#10, @joerg wrote:

Please keep in mind that GCC will effectively pass *all* unknown flags down to ld, which is why this sort of issue appeared in first place. That's also why I am quite strongly opposed against adding more non-driver flags in the front end.

OK. I understand your argument. Make sense!
What about updating the error message to say something like "Passing linker flags without -Wl, is unsupported" ?
It could simplify the work of porting.

[joerg]

Well, I'm not sure if the additional logic for what really helps that much. Consider just misspelling an option which IMHO is the much more common case.

[rnk]

In D4393#5, @joerg wrote:

I'm against this. I haven't seen enough evidence of use to justify it, other options like -R are certainly a lot more popular...

FWIW I'm in favor. -z is not uncommon in my experience.

That said, a principled way to handle this would be to forward exactly the set of flags that gcc documents:
https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html#Link-Options

If you submit a patch to gcc to document -z as a linker option, then we can add it to clang and be done with it. =D

[echristo]

Sadly, I think if gcc documents it we should go ahead and copy that behavior. It's a bit annoying, but the precedent is there and passing it through doesn't harm anything or make it noticeably more difficult to handle.

[bubbles231]

Update patch so it applies with current revision.

[joerg]

I disagree with echristo here. We arrived in this situation because GCC never checked. So this should provide at least a warning.

[bubbles231]

Add a warning whenever someone passes -z alone.

[joerg]

Good enough for me.

[bubbles231]

Unfortunately I have to make a couple of changes before committing. I will
get it done first thing tomorrow.

[bubbles231]

Update patch to be compatible with latest revision.

[rnk]

include/clang/Basic/DiagnosticDriverKinds.td

122 ↗

(On Diff #11438)

Maybe "linker argument %0 should be escaped with -Wl, and commas"? Specifically, I think many users won't realize that they need to escape the second space after -z.

Alternatively, I would be happy if we removed this diagnostic, but I know Joerg won't. My preferred resolution is, again, that we ask GCC to document -z as a linker option and then we support it without equivocation.

include/clang/Driver/Options.td

312

This can be def z :. For -Z we use Z_Flag and Z_Joined because I think they have different semantics.

lib/Driver/Tools.cpp

207

Why do this check this way instead of .matches(options::OPT_z)?

209

Why not A.claim(); A.render(Args, CmdArgs)? Then you can split this whole clause into it's own 'else if' bucket.

7585

ditto, why not .matches?

[bubbles231]

Thanks for the information. I will make those changes.

[bubbles231]

Make the requested changes.

[rnk]

lgtm with suggested tweaks.

lib/Driver/Tools.cpp

206

I was just being terse, I didn't mean to literally put them on the same line. :)

7583–7586

Please revert this part of the change. There is no reason we should forward -z to the MSVC linker.

[bubbles231]

Made requested changes.

[bubbles231]

Committed the changes: http://reviews.llvm.org/rL213198. I am now closing this revision.

include/clang/Driver/Options.td

 def Xanalyzer : Separate<["-"], "Xanalyzer">,
   HelpText<"Pass <arg> to the static analyzer">, MetaVarName<"<arg>">;
 def Xarch__ : JoinedAndSeparate<["-"], "Xarch_">, Flags<[DriverOption]>;
 def Xassembler : Separate<["-"], "Xassembler">,
   HelpText<"Pass <arg> to the assembler">, MetaVarName<"<arg>">;
 def Xclang : Separate<["-"], "Xclang">,
   HelpText<"Pass <arg> to the clang compiler">, MetaVarName<"<arg>">,
   Flags<[DriverOption, CoreOption]>;
+def z_Flag : Separate<["-"], "z">, Flags<[LinkerInput, RenderAsInput]>,
+  HelpText<"Pass -z <arg> to the linker">, MetaVarName<"<arg>">;
 def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass <arg> to the linker">, MetaVarName<"<arg>">;
 def Xpreprocessor : Separate<["-"], "Xpreprocessor">,
   HelpText<"Pass <arg> to the preprocessor">, MetaVarName<"<arg>">;
 def X_Flag : Flag<["-"], "X">;
 def X_Joined : Joined<["-"], "X">;
 def Z_Flag : Flag<["-"], "Z">;
 def Z_Joined : Joined<["-"], "Z">;

lib/Driver/Tools.cpp

if (Dirs.empty()) { // Trailing colon.
     }
   }
 }
 
 static void AddLinkerInputs(const ToolChain &TC,
                             const InputInfoList &Inputs, const ArgList &Args,
                             ArgStringList &CmdArgs) {
   const Driver &D = TC.getDriver();
+  StringRef z_Flag("z");
 
   // Add extra linker input arguments which are not treated as inputs
   // (constructed via -Xarch_).
   Args.AddAllArgValues(CmdArgs, options::OPT_Zlinker_input);
 
   for (const auto &II : Inputs) {
     if (!TC.HasNativeLLVMSupport()) {
       // Don't try to pass LLVM inputs unless we have native support.
for (const auto &II : Inputs) {
     // Otherwise, this is a linker input argument.
     const Arg &A = II.getInputArg();
 
     // Handle reserved library options.
     if (A.getOption().matches(options::OPT_Z_reserved_lib_stdcxx))
       TC.AddCXXStdlibLibArgs(Args, CmdArgs);
     else if (A.getOption().matches(options::OPT_Z_reserved_lib_cckext))
       TC.AddCCKextLibArgs(Args, CmdArgs);
-    else
+    else {
+      // Pass -z prefix for gcc linker compatibility.
+      if (A.getOption().getName().equals(z_Flag))
+        CmdArgs.push_back("-z");
        A.renderAsInput(Args, CmdArgs);
     }
+  }
 
   // LIBRARY_PATH - included following the user specified library paths.
   //                and only supported on native toolchains.
   if (!TC.isCrossCompiling())
     addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
 }
 
 /// \brief Determine whether Objective-C automated reference counting is

 }
 
 void visualstudio::Link::ConstructJob(Compilation &C, const JobAction &JA,
                                       const InputInfo &Output,
                                       const InputInfoList &Inputs,
                                       const ArgList &Args,
                                       const char *LinkingOutput) const {
   ArgStringList CmdArgs;
+  StringRef z_Flag("z");
 
   if (Output.isFilename()) {
     CmdArgs.push_back(Args.MakeArgString(std::string("-out:") +
                                          Output.getFilename()));
   } else {
     assert(Output.isNothing() && "Invalid output.");
   }
 
void visualstudio::Link::ConstructJob(Compilation &C, const JobAction &JA,
 
   Args.AddAllArgValues(CmdArgs, options::OPT_l);
   Args.AddAllArgValues(CmdArgs, options::OPT__SLASH_link);
 
   // Add filenames immediately.
   for (const auto &Input : Inputs)
     if (Input.isFilename())
       CmdArgs.push_back(Input.getFilename());
-    else
+    else {
+      // Pass -z prefix for gcc linker compatibility.
+      if (Input.getInputArg().getOption().getName().equals(z_Flag))
+        CmdArgs.push_back("-z");
        Input.getInputArg().renderAsInput(Args, CmdArgs);
+    }
 
   const char *Exec =
     Args.MakeArgString(getToolChain().GetProgramPath("link.exe"));
   C.addCommand(new Command(JA, *this, Exec, CmdArgs));
 }
 
 void visualstudio::Compile::ConstructJob(Compilation &C, const JobAction &JA,
                                          const InputInfo &Output,

test/Driver/Xlinker-args.c

 // Check that we extract --no-demangle from '-Xlinker' and '-Wl,', since that
 // was a collect2 argument.
 
 // RUN: %clang -target i386-apple-darwin9 -### \
 // RUN:   -Xlinker one -Xlinker --no-demangle \
-// RUN:   -Wl,two,--no-demangle,three -Xlinker four %s 2> %t
+// RUN:   -Wl,two,--no-demangle,three -Xlinker four -z five %s 2> %t
 // RUN: FileCheck -check-prefix=DARWIN < %t %s
 //
 // RUN: %clang -target x86_64-pc-linux-gnu -### \
 // RUN:   -Xlinker one -Xlinker --no-demangle \
-// RUN:   -Wl,two,--no-demangle,three -Xlinker four %s 2> %t
+// RUN:   -Wl,two,--no-demangle,three -Xlinker four -z five %s 2> %t
 // RUN: FileCheck -check-prefix=LINUX < %t %s
 //
 // DARWIN-NOT: --no-demangle
-// DARWIN: "one" "two" "three" "four"
-// LINUX: "--no-demangle" "one" "two" "three" "four"
+// DARWIN: "one" "two" "three" "four" "-z" "five"
+// LINUX: "--no-demangle" "one" "two" "three" "four" "-z" "five"
 
 // Check that we forward '-Xlinker' and '-Wl,' on Windows.
 // RUN: %clang -target i686-pc-win32 -### \
-// RUN:   -Xlinker one -Wl,two %s 2>&1 | \
-// RUN:   FileCheck -check-prefix=WIN %s
+// RUN:   -Xlinker one -Wl,two -z three %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=WIN %s 
 // WIN: link.exe
 // WIN: "one"
 // WIN: "two"