[Driver] Revert D139717 and add -Xparser instead

[MaskRay]

Summary

Some macOS projects use -Xparser even if it leads to a
-Wunused-command-line-argument warning. It doesn't justify adding a broad Joined
-X (IgnoredGCCCompat) as GCC doesn't really support these arbitrary -X
options.

Note: -Xcompiler foo is a GNU libtool option, not a driver option.
It is misused by some ChromeOS packages (but not by Gentoo).
Keep it for a while.

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

[MaskRay]

.

[davide]

Fine if Roy is happy. If we find another thing that break, we'll let you know. Please wait for him to comment.

[manojgupta]

Without -Xcompiler, ChromeOS code will break. It may not be supported by GCC but it is supported in some other compilers like Cuda and a few others if you search. Also being supported by libtool makes it more important to keep it working.

[MaskRay]

In D140224#4001748, @manojgupta wrote:

Without -Xcompiler, ChromeOS code will break. It may not be supported by GCC but it is supported in some other compilers like Cuda and a few others if you search. Also being supported by libtool makes it more important to keep it working.

Can you give more information how ChromeOS will break? My understanding is that libtool confumses -Xcompiler foo and passes foo to Clang. Clang Driver is not supposed to get -Xcompiler from libtool.
But I don't know how CUDA comes into play. It's possible the -Xcompiler interaction is not intended. If there is indeed problematic but widespread misuse of -Xcompiler, I can certainly add an ignored -Xcompiler, but I want to understand the problem first.

[manojgupta]

Here are a few instances of Xcompiler usage for a non-exhaustive search (I can't look inside package tarballs if they are using it ):

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/main/net-vpn/openvpn/openvpn-2.4.4.ebuild#74
https://chromium.googlesource.com/external/googleappengine/python/+/bedccc3dd4178880371cdf44064b222d82a5f30d/lib/distutils/distutils/extension.py#219

We already ignore Xcompiler for GCC in our compiler wrapper (https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/main/compiler_wrapper/gcc_flags.go), and we can do the same for clang as well.
So in theory, we'll be ok with the removal but it is annoying for sure to deal with.

[MaskRay]

In D140224#4001881, @manojgupta wrote:

Here are a few instances of Xcompiler usage for a non-exhaustive search (I can't look inside package tarballs if they are using it ):

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/main/net-vpn/openvpn/openvpn-2.4.4.ebuild#74
https://chromium.googlesource.com/external/googleappengine/python/+/bedccc3dd4178880371cdf44064b222d82a5f30d/lib/distutils/distutils/extension.py#219

We already ignore Xcompiler for GCC in our compiler wrapper (https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/main/compiler_wrapper/gcc_flags.go), and we can do the same for clang as well.
So in theory, we'll be ok with the removal but it is annoying for sure to deal with.

This seems ChromeOS specific, not from Gentoo (according to grep -rsin "Xcompiler" --include="*.ebuild" done by sam AT gentoo).

use static && append-ldflags -Xcompiler -static

This should just use -static, instead of -Xcompiler -static.

lib/distutils/distutils/extension.py#219 -Xcompiler seems unnecessary and I am unsure why it has it.

If ChromeOS needs time for migration, I think -Xcompiler can be temporarily ignored.

[manojgupta]

If ChromeOS needs time for migration, I think -Xcompiler can be temporarily ignored.

If you can wait for a few weeks, that'd be great. We are already fighting with a large number of ToT issues. And this change makes our builds fail very early.

[MaskRay]

I'll reject [-\Xparser for a while as well. This is a valid amendment to D139717 , so I don't think it needs more approval.

[davide]

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

[MaskRay]

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

[davide]

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

[rsundahl]

I'll reject [-\Xparser for a while as well. This is a valid amendment to D139717 , so I don't think it needs more approval.

We have projects that are failing because of -Xlinker and I'm not too excited about walking through every project we have to find more examples. Can we please have a reprieve on deprecating this until every project in the world that uses these flags that have existed for a very long time can have a chance to at least get timely notification? Why is this any different than anything else that gets deprecated?

[MaskRay]

In D140224#4014234, @davide wrote:

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that such -X* has -Wunused-command-line-argument warning for many many years.
I'm not sure how is considered insufficient.

"Working around 3 cases creates more complexity than it fixes." the number isn't that high. By enumerating the misuse, we have a valid path to remove all workarounds as misuses are fixed.
This made some forward progress.

[davide]

In D140224#4014245, @MaskRay wrote:

In D140224#4014234, @davide wrote:

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that such -X* has -Wunused-command-line-argument warning for many many years.
I'm not sure how is considered insufficient.

"Working around 3 cases creates more complexity than it fixes." the number isn't that high. By enumerating the misuse, we have a valid path to remove all workarounds as misuses are fixed.
This made some forward progress.

You can't just remove options willy-nilly. This is not how drivers work. The warning says "unused", it doesn't say "it goes away".
If we want to provide a path forward, we first need to reinstate this, then change the warning, then remove (in 1 year or something).
That's how transitions work.

HTH.

[MaskRay]

In D140224#4014243, @rsundahl wrote:

I'll reject [-\Xparser for a while as well. This is a valid amendment to D139717 , so I don't think it needs more approval.

We have projects that are failing because of -Xlinker and I'm not too excited about walking through every project we have to find more examples. Can we please have a reprieve on deprecating this until every project in the world that uses these flags that have existed for a very long time can have a chance to at least get timely notification? Why is this any different than anything else that gets deprecated?

Thank you for your reply! If there are more issues -Xlinker/-Xparser/-Xcctests, ok, I think I'm fine with a Joined -X, but ideally we can figure out a way to apply that only to Apple platforms.

(FWIW I feel sad when I made valid objection to D139717, but that patch landed very soon after your colleague approved it, leaving me no time to object.
My other objection included the insufficient commit message -Xfoo instead of what's actually used, internal builds which seems to hint an internal workaround, where the tests are located)

[MaskRay]

In D140224#4014246, @davide wrote:

In D140224#4014245, @MaskRay wrote:

In D140224#4014234, @davide wrote:

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that such -X* has -Wunused-command-line-argument warning for many many years.
I'm not sure how is considered insufficient.

"Working around 3 cases creates more complexity than it fixes." the number isn't that high. By enumerating the misuse, we have a valid path to remove all workarounds as misuses are fixed.
This made some forward progress.

You can't just remove options willy-nilly. This is not how drivers work. The warning says "unused", it doesn't say "it goes away".
If we want to provide a path forward, we first need to reinstate this, then change the warning, then remove (in 1 year or something).
That's how transitions work.

HTH.

I almost agree with you, but only for actually-used options which do real work instead of ignored options.
If it is pure compatibility option, I wish that there is public references instead of pure internal build uses.
For Joined -X I am unsure I want to take your opinion.

Neither sourcegraph nor search/Debian Code Search shows anything about -Xcctests.
Separate -Xlinker still exists: not removed by this patch. -Xlinker= is invalid. Note that many references on a code search website reveal libtool uses instead of compiler driver uses.
If your internal build uses many -X* forms, I am fine with ignoring them. If there are so many that you want to ignore -X*, I think I am fine as well, but ideally restricted to Apple platforms (ideally show some external usage examples).

[davide]

In D140224#4014256, @MaskRay wrote:

In D140224#4014243, @rsundahl wrote:

I'll reject [-\Xparser for a while as well. This is a valid amendment to D139717 , so I don't think it needs more approval.

We have projects that are failing because of -Xlinker and I'm not too excited about walking through every project we have to find more examples. Can we please have a reprieve on deprecating this until every project in the world that uses these flags that have existed for a very long time can have a chance to at least get timely notification? Why is this any different than anything else that gets deprecated?

Thank you for your reply! If there are more issues -Xlinker/-Xparser/-Xcctests, ok, I think I'm fine with a Joined -X, but ideally we can figure out a way to apply that only to Apple platforms.

(FWIW I feel sad when I made valid objection to D139717, but that patch landed very soon after your colleague approved it, leaving me no time to object.
My other objection included the insufficient commit message -Xfoo instead of what's actually used, internal builds which seems to hint an internal workaround, where the tests are located)

It's not an internal workaround.

In D140224#4014291, @MaskRay wrote:

In D140224#4014246, @davide wrote:

In D140224#4014245, @MaskRay wrote:

In D140224#4014234, @davide wrote:

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that such -X* has -Wunused-command-line-argument warning for many many years.
I'm not sure how is considered insufficient.

"Working around 3 cases creates more complexity than it fixes." the number isn't that high. By enumerating the misuse, we have a valid path to remove all workarounds as misuses are fixed.
This made some forward progress.

You can't just remove options willy-nilly. This is not how drivers work. The warning says "unused", it doesn't say "it goes away".
If we want to provide a path forward, we first need to reinstate this, then change the warning, then remove (in 1 year or something).
That's how transitions work.

HTH.

I almost agree with you, but only for actually-used options which do real work instead of ignored options.
If it is pure compatibility option, I wish that there is public references instead of pure internal build uses.
For Joined -X I am unsure I want to take your opinion.

Neither sourcegraph nor search/Debian Code Search shows anything about -Xcctests.

Debian code search and source graphs aren't the universe. There's a lot of code in the world that doesn't get published for a variety of reasons.
The gain of this change is very little compared to the pain introduced. Let's give people a window where they can transition, then screw them over. not the other way around.

Best,

D

Separate -Xlinker still exists: not removed by this patch. -Xlinker= is invalid. Note that many references on a code search website reveal libtool uses instead of compiler driver uses.
If your internal build uses many -X* forms, I am fine with ignoring them. If there are so many that you want to ignore -X*, I think I am fine as well, but ideally restricted to Apple platforms (ideally show some external usage examples).

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;
+// Used by some macOS projects. IgnoredGCCCompat is a misnomer since GCC doesn't allow it.
+def : Flag<["-"], "Xparser">, IgnoredGCCCompat;
+// FIXME -Xcompiler is misused by some ChromeOS packages. Remove it after a while.
+def : Flag<["-"], "Xcompiler">, 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/Driver/warn-Xparser.c

+/// Some macOS projects use -Xparser.
+// RUN: %clang -c -Xparser %s 2>&1 | FileCheck %s
+
+// CHECK: warning: argument unused during compilation: '-Xparser' [-Wunused-command-line-argument]
+
+void f(void) {}

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

-// RUN: %clang -c %s -o /dev/null -Xfoo < %s 2>&1 | FileCheck --check-prefix=CHECK_STANDALONE_FOO %s
-// RUN: %clang -c %s -o /dev/null -Xfoo=bar 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) {}