This is an archive of the discontinued LLVM Phabricator instance.

GCC compatibility: pass -z linker options to the linker
ClosedPublic

Authored by bubbles231 on Jul 4 2014, 8:11 AM.

Details

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

Diff Detail

Event Timeline

bubbles231 updated this revision to Diff 11088.Jul 4 2014, 8:11 AM
bubbles231 retitled this revision from to GCC compatibility: pass -z linker options to the linker.
bubbles231 updated this object.
bubbles231 edited the test plan for this revision. (Show Details)
bubbles231 added reviewers: rnk, rafael.
rafael edited edge metadata.Jul 4 2014, 8:14 AM
rafael added a subscriber: Unknown Object (MLST).

adding cfe-commits

joerg added a subscriber: joerg.Jul 4 2014, 8:38 AM

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

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 added a comment.Jul 4 2014, 9:21 AM

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 added a comment.Jul 4 2014, 9:24 AM

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.

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 added a comment.Jul 5 2014, 1:20 PM

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 edited edge metadata.Jul 7 2014, 12:03 PM
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

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 updated this revision to Diff 11350.Jul 13 2014, 7:25 AM
bubbles231 edited edge metadata.

Update patch so it applies with current revision.

joerg added a comment.Jul 13 2014, 9:15 AM

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

bubbles231 updated this revision to Diff 11383.Jul 14 2014, 7:36 AM

Add a warning whenever someone passes -z alone.

Good enough for me.

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

bubbles231 updated this revision to Diff 11438.Jul 15 2014, 6:30 AM

Update patch to be compatible with latest revision.

rnk added inline comments.Jul 15 2014, 1:33 PM
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
305

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
212

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

214

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

7638

ditto, why not .matches?

Thanks for the information. I will make those changes.

bubbles231 updated this revision to Diff 11480.Jul 15 2014, 5:48 PM

Make the requested changes.

rnk accepted this revision.Jul 15 2014, 6:03 PM
rnk edited edge metadata.

lgtm with suggested tweaks.

lib/Driver/Tools.cpp
208

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

7633–7636

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

This revision is now accepted and ready to land.Jul 15 2014, 6:03 PM
bubbles231 updated this revision to Diff 11482.Jul 15 2014, 6:24 PM
bubbles231 edited edge metadata.

Made requested changes.

bubbles231 closed this revision.Jul 16 2014, 2:28 PM

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