Revert "[Driver] Remove Joined -X"

[rsundahl]

Summary

This change is breaking internal builds. We use the -Xfoo pattern but can
now no longer manage whether we allow an unused -Xfoo option to pass as a
warning or promote it to an error.

This reverts commit 98615fd376cea15af21e120e0e3ffa5ba68c2b6d.

[lebedev.ri]

This is missing a test, like the original commit mentioned.
Why can you not just use the the split variant, -X clang ...?

[davide]

In D139717#3984648, @lebedev.ri wrote:

This is missing a test, like the original commit mentioned.
Why can you not just use the the split variant, -X clang ...?

This breaks many projects internally. There's no real complexity to keep this around. Removing is more annoying than anything else.
I agree on the test, that has to be added.

[manojgupta]

The removal is also breaking ChromeOS builds which use -Xpattern in some cases.

[MaskRay]

In D139717#3986227, @manojgupta wrote:

The removal is also breaking ChromeOS builds which use -Xpattern in some cases.

Do you have a justifying example showing which -Xpattern is used? It needs to do useful work, instead of just using an option which happened to be ignored (and probably not ignored by GCC).

[MaskRay]

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.
Is it possible to clean up the previously-ignored-with-a-warning -Xfoo?

[manojgupta]

We use -Xcompiler and -Xlinker which are passed in programs and they raise error now.

[rsundahl]

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.

We use -Xparser

Is it possible to clean up the previously-ignored-with-a-warning -Xfoo?

Possibly. Maybe preferably. Builds are breaking now though. Dependent projects would have to be notified and changes scheduled. Was there a diff that we should have been part of? It looks like at least two of us are surprised with downstream consequences.

[davide]

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.
Is it possible to clean up the previously-ignored-with-a-warning -Xfoo?

@MaskRay -- we can discuss options but we can't just "kill" options willy-nilly just because you don't use them.
A better way of doing this would be marking them as deprecated, wait one release and then remove. Going cold turkey breaks clients you never heard of, and you didn't even warn.

Let's add a test and revert this patch, then we can continue the discussion on how to phase this out.

[MaskRay]

In D139717#3988902, @manojgupta wrote:

We use -Xcompiler and -Xlinker which are passed in programs and they raise error now.

Sorry, I do not understand. clang -Xlinker --verbose a.c works well. -Xcompiler is invalid in GCC. How do your programs use -Xlinker or -Xcompiler.
If -Xcompiler foo is used similar to -Xclang foo, it seems very wrong to silently ignore -Xcompiler as foo will be seen as a positional argument...

[MaskRay]

In D139717#3991765, @rsundahl wrote:

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.

We use -Xparser

There are two possibilities.

First, your downstream projects incorrectly pass a previously-ignored -Xparser. Then it seems the right call is to remove it.

Second, there are many projects doing -Xparser and it was intentionally ignored. Another program would analyze -Xparser.

For the second case, we can add an ignored Flag called -Xparser, with a comment which tool uses it.
We should not call it IgnoreGCCCompat as GCC doesn't recognize -Xparser.

We should try avoiding a broad -X*. If you want to carve out a larger namespace, say, -Xparser*, it may be fine as well. But calling it IgnoreGCCCompat with some vendor-specific undocumented thing seems not right.

Is it possible to clean up the previously-ignored-with-a-warning -Xfoo?

Possibly. Maybe preferably. Builds are breaking now though. Dependent projects would have to be notified and changes scheduled. Was there a diff that we should have been part of? It looks like at least two of us are surprised with downstream consequences.

See the previous paragraph. And we need a clang/test/Driver test to catch accidental removal from other contributors.

[manojgupta]

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

[davide]

In D139717#3997193, @MaskRay wrote:

In D139717#3991765, @rsundahl wrote:

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.

We use -Xparser

There are two possibilities.

First, your downstream projects incorrectly pass a previously-ignored -Xparser. Then it seems the right call is to remove it.

It is not. You first deprecate and then you remove. This commit needs to be reverted.

HTH,

D

[rsundahl]

Added the test warn-not-error-Xfoo

[rsundahl]

I added a test so we can catch any regression. If we choose to deprecate then we can modify the test to watch for the "is deprecated" message that should be asserted for "some time" appropriate. Thank you all for your input.

[davide]

LGTM

[MaskRay]

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

[MaskRay]

In D139717#3999098, @rsundahl wrote:

Added the test warn-not-error-Xfoo

-Xfoo leads to a warning (expected) which is weird. Can you change it to a form which actually reflects how -X is used? (aka -Xparser)

I am not sure adding arbitrary -X* which leads to a -Wunused-command-line-argument warning is useful. It certainly leads to confusion why this is supported at all, if the needs are just -Xparser.

[davide]

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. Noone is objecting about that.

[MaskRay]

In D139717#4001688, @davide wrote:

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. Noone is objecting about that.

-Xparser has always been leading to such a warning: warning: argument unused during compilation: '-Xparser' [-Wunused-command-line-argument], perhaps since forever when the option was added in the first place?
The message is different from ... deprecation ... but isn't it sufficient as well?

[davide]

In D139717#4001702, @MaskRay wrote:

In D139717#4001688, @davide wrote:

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. Noone is objecting about that.

-Xparser has always been leading to such a warning: warning: argument unused during compilation: '-Xparser' [-Wunused-command-line-argument], perhaps since forever when the option was added in the first place?
The message is different from ... deprecation ... but isn't it sufficient as well?

Yes. it is. "unused" doesn't mean "will go away".
Sometimes if you pass linker flags to the compiler you get the same "unused" warning. Noone expects them to go away.

Hope this helps.

[MaskRay]

In D139717#4001704, @davide wrote:

In D139717#4001702, @MaskRay wrote:

In D139717#4001688, @davide wrote:

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. Noone is objecting about that.

-Xparser has always been leading to such a warning: warning: argument unused during compilation: '-Xparser' [-Wunused-command-line-argument], perhaps since forever when the option was added in the first place?
The message is different from ... deprecation ... but isn't it sufficient as well?

Yes. it is. "unused" doesn't mean "will go away".
Sometimes if you pass linker flags to the compiler you get the same "unused" warning. Noone expects them to go away.

Hope this helps.

Sorry, it doesn't help. A compiler option used a linker option leading to a "unused" warning is very different from this case.
I can understand that many CFLAGS options may end up in linking with a -Wunused-command-line-argument warning.
For practicality we must support them, as they are used in other phases.

In your case, at least as stated in the commit message, This change is breaking internal builds. (This message, if not from an affiliation with much contribution to Clang, might be conceived as weird by a casual contributor.)
This is only for -Xparser. Then why can't you just add a def : Flag<["-"], "Xparser">?

It seems that GCC < 4.6 reports g++: unrecognized option '-Xfoo' but exit with 0 while GCC >= 4.6 reports g++: error: unrecognized option '-Xaaa' and exits with 1.
I don't think GCC ever supports -Xcompiler or -Xparser, so IgnoredGCCCompat is not justified.

clang/include/clang/Driver/Options.td

def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
 def Xoffload_linker : JoinedAndSeparate<["-"], "Xoffload-linker">,
   HelpText<"Pass <arg> to the offload linkers or the ones idenfied by -<triple>">,
   MetaVarName<"<triple> <arg>">, Group<Link_Group>;
 def Xpreprocessor : Separate<["-"], "Xpreprocessor">, Group<Preprocessor_Group>,
   HelpText<"Pass <arg> to the preprocessor">, MetaVarName<"<arg>">;
 def X_Flag : Flag<["-"], "X">, Group<Link_Group>;
+def X_Joined : Joined<["-"], "X">, IgnoredGCCCompat;
 def Z_Flag : Flag<["-"], "Z">, Group<Link_Group>;
 def all__load : Flag<["-"], "all_load">;
 def allowable__client : Separate<["-"], "allowable_client">;
 def ansi : Flag<["-", "--"], "ansi">, Group<CompileOnly_Group>;
 def arch__errors__fatal : Flag<["-"], "arch_errors_fatal">;
 def arch : Separate<["-"], "arch">, Flags<[NoXarchOption]>;
 def arch__only : Separate<["-"], "arch_only">;
 def autocomplete : Joined<["--"], "autocomplete=">;

clang/test/Misc/warn-not-error-Xfoo.c

+// RUN: %clang -c -Xfoo %s 2>&1 | FileCheck --check-prefix=CHECK_STANDALONE_FOO %s
+// RUN: %clang -c -Xfoo=bar %s 2>&1 | FileCheck --check-prefix=CHECK_JOINED_FOO %s
+
+// This test ensures that we only warn on -X<unknown> and -X<unknown=value>
+// in case it is used downstream. If we error, we can't ignore it and some
+// use of these (ignored) flags are in legacy use.
+// TODO: Deprecate with timebox warning so consumers can respond.
+
+// CHECK_STANDALONE_FOO: warning: argument unused during compilation: '-Xfoo' [-Wunused-command-line-argument]
+// CHECK_JOINED_FOO: warning: argument unused during compilation: '-Xfoo=bar' [-Wunused-command-line-argument]
+
+// CHECK-NOT: clang{.*}: error: unknown argument:
+
+void f(void) {}