Make the driver accept all four variants of the target option

[enefaim]

Summary

Using clang as a cross-compiler with the 'target' option could be confusing
for those inexperienced in the realm of cross compiling.
This patch would allow the use of all these four variants of the target option:
-target <triple>
--target <triple>
-target=<triple>
--target=<triple>

[rengolin]

I have to say I'm not a big fan of multiple ways of doing the same thing, and I don't know where the Clang developers are going with the way we use flags. I know we had a few iterations in the past do define what to do and where to go, but that was too long ago.

Maybe you could start a thread on cfe-dev@ and see where this goes.

[meadori]

Perhaps I am missing something, but how does having four variants for the option make it less confusing as a cross compiler? Do we have a convention on which clang long options have '=' forms?

[richard.barton.arm]

I think that having -target=<triple> and --target <triple> only is awkward.
Why have two version of the same option with different rules on whether they
accept = or spaced arguments?

I think Gabor's patch makes sense as it does not break any backwards
compatibility within clang itself and will only help people using the tool.

The patch itself leaves some duplication in the Options.td file. Can't we
combine "target_legacy_spelling_EQ" with "target"? And perhaps rename them
both to just "target" and "target_EQ"?

Thanks
Rich

[rengolin]

In D7730#126290, @richard.barton.arm wrote:

I think that having -target=<triple> and --target <triple> only is awkward.
Why have two version of the same option with different rules on whether they
accept = or spaced arguments?

Hi Richard,

There's a consensus in the Unix world that only two such syntaxes should be supported:

-key value
--key=value

We only accept those two, and not the other two. This is intentional.

I think Gabor's patch makes sense as it does not break any backwards
compatibility within clang itself and will only help people using the tool.

I agree that backwards compatibility is important, but forwards compatibility is equally so. Every new flag you add will create a backwards compatibility problem for you in the future, as well as forcing us to accept multiple flags on all our other tools (such as llc, lli, etc).

I'm curious as to why you guys need this change. Maybe, if you explain the reasons, we could find another, less problematic solution.

cheers,
--renato

[richard.barton.arm]

There's a consensus in the Unix world that only two such syntaxes should be
supported:

-key value
--key=value

We only accept those two, and not the other two. This is intentional.

Point taken, but there are counterexamples. A very quick play in my terminal shows that diff accepts the option --ifdef with either a space or = separating the argument. Same for grep and --exclude, emacs and --cursor-color.

Clang is no different either:

-mcpu=, -fdiagnostics-color= are both valid.

--include-prefix (alias for gcc -iprefix) only accepts a space as argument separator. Similarly, --output (synonym for -o), --print-file-name, --sysroot.

--mhwdiv is available as --mhwdiv=arm, --mhwdiv arm and -mhwdiv=arm, but not as -mhwdiv arm!

I'm curious as to why you guys need this change. Maybe, if you explain the
reasons, we could find another, less problematic solution.

"Need" is too strong here. Our concern is out-of-box usability to people unfamiliar with the tool. If a user takes clang and passes --target arm-none-eabi or -target=arm-none-eabi they get error messages like:

clang-3.7: error: unsupported option '--target'
clang-3.7: error: no such file or directory: 'arm-none-eabi'

or

clang-3.7: error: unknown argument: '-target=arm-none-eabi'

which is pretty poor.

It seems to me that given clang is not consistent with its application of these unix rules and that there are no gcc compatibility issues to consider, this would be a simple way to improve the situation.

Ta
Rich

[rengolin]

In D7730#126353, @richard.barton.arm wrote:

Point taken, but there are counterexamples. A very quick play in my terminal shows that diff accepts the option --ifdef with either a space or = separating the argument. Same for grep and --exclude, emacs and --cursor-color.

If the others are not following the "standard", let's not break what is.

-mcpu=, -fdiagnostics-color= are both valid.

These are GCC's fault, there isn't much we can do.

--mhwdiv is available as --mhwdiv=arm, --mhwdiv arm and -mhwdiv=arm, but not as -mhwdiv arm!

I'd remove the non-conforming options. How does GCC behave in those cases?

"Need" is too strong here. Our concern is out-of-box usability to people unfamiliar with the tool. If a user takes clang and passes --target arm-none-eabi or -target=arm-none-eabi they get error messages like:

clang-3.7: error: unsupported option '--target'
clang-3.7: error: no such file or directory: 'arm-none-eabi'

or

clang-3.7: error: unknown argument: '-target=arm-none-eabi'

which is pretty poor.

I agree, but we should fix the error messages, and not add more complexity. We already have infrastructure in Clang to suggest the appropriate syntax, we should use that instead.

It seems to me that given clang is not consistent with its application of these unix rules and that there are no gcc compatibility issues to consider, this would be a simple way to improve the situation.

We don't "have" to be consistent to anything, we just *want* to be. The ones we chose to follow on Unix systems are the Unix and GCC standards, where it fits. My view is that everything else that is neither, should fit into one or the other.

I certainly don't speak for everyone, but for this specific case, I think we should fix the diagnostics on -target, instead of accepting more options.

cheers,
--renato

[echristo]

Let's go with it. I don't see a reason not to. The only (somewhat silly) objection I was thinking was that --target <triple> feels like the configure option. But that's not necessarily a bad thing and I like it more than --target= anyhow :)

-eric

[rsmith]

By coincidence, I happened to run into the fact that we don't support
--target earlier today. We have a lot of truly weird baggage in our
command-line syntax, but I think we should be striving to minimize it. Is
there a justification for supporting one of "--foo bar" and "--foo=bar" but
not the other, for *any* of our options with arguments?

(I find it especially weird that our TableGen option mechanism has native
support for handling "-Ifoo" and "-I foo" as the same option, but not for
the more common case of "--blah foo" and "--blah=foo".)

[enefaim]

Hi Eric,

Thanks for the review. Could you land it for me, please?
I don't have commit rights.

Thanks,
Gabor

[richard.barton.arm]

Hi Gabor

Do you think the patch could be further refined?

Can "target_legacy_spelling_EQ" not be combined with "target"?

Ta
Rich

[enefaim]

Hi Richard,

I uploaded a new patch with the changes you requested.

Thanks,
Gabor

[rengolin]

LGTM.

[enefaim]

In D7730#137539, @rengolin wrote:

LGTM.

Hi Renato,

Thanks for the review. Could you land the patch for me?
I don't have commit rights.

Thanks,
Gabor

[rengolin]

r231787

include/clang/Driver/Options.td

            "system header.">;
 def : Separate<["--"], "system-header-prefix">, Alias<system_header_prefix>;
 def no_system_header_prefix : Joined<["--"], "no-system-header-prefix=">,
   Group<clang_i_Group>, Flags<[CC1Option]>, MetaVarName<"<prefix>">,
   HelpText<"Treat all #include paths starting with <prefix> as not including a "
            "system header.">;
 def : Separate<["--"], "no-system-header-prefix">, Alias<no_system_header_prefix>;
 def s : Flag<["-"], "s">;
-def target : Joined<["--"], "target=">, Flags<[DriverOption, CoreOption]>,
+def target : Joined<["-", "--"], "target=">, Flags<[DriverOption, CoreOption]>,
   HelpText<"Generate code for the given target">;
 def gcc_toolchain : Joined<["--"], "gcc-toolchain=">, Flags<[DriverOption]>,
   HelpText<"Use the gcc toolchain at the given directory">;
 def time : Flag<["-"], "time">,
   HelpText<"Time individual commands">;
 def traditional_cpp : Flag<["-", "--"], "traditional-cpp">, Flags<[CC1Option]>,
   HelpText<"Enable some traditional CPP emulation">;
 def traditional : Flag<["-", "--"], "traditional">;

           AliasArgs<["v4"]>;
 def mv5 : Flag<["-"], "mv5">, Group<m_hexagon_Features_Group>, Alias<march_EQ>,
           AliasArgs<["v5"]>;
 
 // These are legacy user-facing driver-level option spellings. They are always
 // aliases for options that are spelled using the more common Unix / GNU flag
 // style of double-dash and equals-joined flags.
 def gcc_toolchain_legacy_spelling : Separate<["-"], "gcc-toolchain">, Alias<gcc_toolchain>;
-def target_legacy_spelling : Separate<["-"], "target">, Alias<target>;
+def target_legacy_spelling : Separate<["-", "--"], "target">, Alias<target>;
 
 // Special internal option to handle -Xlinker --no-demangle.
 def Z_Xlinker__no_demangle : Flag<["-"], "Z-Xlinker-no-demangle">,
     Flags<[Unsupported, NoArgumentUnused]>;
 
 // Special internal option to allow forwarding arbitrary arguments to linker.
 def Zlinker_input : Separate<["-"], "Zlinker-input">,
     Flags<[Unsupported, NoArgumentUnused]>;

test/Driver/target.c

 // RUN: %clang -no-canonical-prefixes --target=unknown-unknown-unknown -c %s \
 // RUN:   -o %t.o -### 2>&1 | FileCheck %s
 //
 // Ensure we get a crazy triple here as we asked for one.
 // CHECK: Target: unknown-unknown-unknown
 //
 // Also check that the legacy spelling works.
 // RUN: %clang -no-canonical-prefixes -target unknown-unknown-unknown -c %s \
 // RUN:   -o %t.o -### 2>&1 | FileCheck %s
+//
+// RUN: %clang -no-canonical-prefixes -target=unknown-unknown-unknown -c %s \
+// RUN:   -o %t.o -### 2>&1 | FileCheck %s
+//
+// RUN: %clang -no-canonical-prefixes --target unknown-unknown-unknown -c %s \
+// RUN:   -o %t.o -### 2>&1 | FileCheck %s