Page MenuHomePhabricator

Make the driver accept all four variants of the target option
ClosedPublic

Authored by enefaim on Feb 18 2015, 10:00 AM.

Details

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>

Diff Detail

Event Timeline

enefaim updated this revision to Diff 20197.Feb 18 2015, 10:00 AM
enefaim retitled this revision from to Make the driver accept all four variants of the target option.
enefaim updated this object.
enefaim edited the test plan for this revision. (Show Details)
enefaim added reviewers: rengolin, kristof.beyls.
enefaim set the repository for this revision to rL LLVM.
enefaim added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.Feb 18 2015, 10:08 AM

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.

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?

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

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

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

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 accepted this revision.Feb 19 2015, 9:34 AM
echristo added a reviewer: echristo.
echristo added a subscriber: 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

This revision is now accepted and ready to land.Feb 19 2015, 9:34 AM
rsmith added a subscriber: rsmith.Feb 19 2015, 5:33 PM

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

Hi Eric,

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

Thanks,
Gabor

Hi Gabor

Do you think the patch could be further refined?

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

Ta
Rich

enefaim updated this revision to Diff 21011.Mar 2 2015, 9:14 AM
enefaim edited edge metadata.
enefaim removed rL LLVM as the repository for this revision.

Hi Richard,

I uploaded a new patch with the changes you requested.

Thanks,
Gabor

rengolin accepted this revision.Mar 10 2015, 4:56 AM
rengolin edited edge metadata.

LGTM.

LGTM.

Hi Renato,

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

Thanks,
Gabor

rengolin closed this revision.Mar 10 2015, 7:01 AM

r231787