Page MenuHomePhabricator

clang-cl: Add "/clang:" pass-through arg support.
ClosedPublic

Authored by neerajksingh on Oct 19 2018, 3:46 PM.

Details

Summary

The clang-cl driver disables access to command line options outside of the
"Core" and "CLOption" sets of command line arguments. This filtering makes it
impossible to pass arguments that are interpreted by the clang driver and not
by either 'cc1' (the frontend) or one of the other tools invoked by the driver.

An example driver-level flag is the '-fno-slp-vectorize' flag, which is
processed by the driver in Clang::ConstructJob and used to set the cc1 flag
"-vectorize-slp". There is no negative cc1 flag or -mllvm flag, so it is not
currently possible to disable the SLP vectorizer from the clang-cl driver.

This change introduces the "/clang:" argument that is available when the
driver mode is set to CL compatibility. This option works similarly to the
"-Xclang" option, except that the option values are processed by the clang
driver rather than by 'cc1'. An example usage is:

clang-cl /clang:-fno-slp-vectorize /O2 test.c

Another example shows how "/clang:" can be used to pass a flag where there is
a conflict between a clang-cl compat option and an overlapping clang driver
option:

clang-cl /MD /clang:-MD /clang:-MF /clang:test_dep_file.dep test.c

In the previous example, the unprefixed /MD selects the DLL version of the msvc
CRT, while the prefixed -MD flag and the -MF flags are used to create a make
dependency file for included headers.

One note about flag ordering: the /clang: flags are concatenated to the end of
the argument list, so in cases where the last flag wins, the /clang: flags
will be chosen regardless of their order relative to other flags on the driver
command line.

Diff Detail

Repository
rC Clang

Event Timeline

neerajksingh created this revision.Oct 19 2018, 3:46 PM
neerajksingh edited the summary of this revision. (Show Details)
hans added a comment.Oct 19 2018, 4:31 PM

I haven't started looking at the code yet.

I'm not completely convinced that we want this. So far we've used the strategy of promoting clang options that are also useful in clang-cl to core options, and if someone wants to use more clang than that, maybe clang-cl isn't the right driver for that use-case.

But I suppose an argument could be made for having an escape hatch from clang-cl if it doesn't add too much complexity to the code.

I'm not sure I'm a fan of calling it /Xdriver: though, because what does it mean - clang-cl is the driver, but the option refers to the clang driver. The natural name would of course be -Xclang but that already means something else. Maybe we could just call it /clang:

rnk added a comment.Oct 22 2018, 10:37 AM

I haven't started looking at the code yet.

I'm not completely convinced that we want this. So far we've used the strategy of promoting clang options that are also useful in clang-cl to core options, and if someone wants to use more clang than that, maybe clang-cl isn't the right driver for that use-case.

But I suppose an argument could be made for having an escape hatch from clang-cl if it doesn't add too much complexity to the code.

Personally, I really want to get out of the role of being a gatekeeper for GCC-style driver flags. I want users to be able to solve their own problems without building clang-cl from scratch or waiting for the next 6 month release.

I'm personally in favor of moving clang-cl to a blacklist model instead of a whitelist model so that we go even further in this direction, but it doesn't address what to do about conflicting or ambiguous options like -MD. An escape hatch like [-/]Xdriver: seems like a useful thing to have when that arises.

I'm not sure I'm a fan of calling it /Xdriver: though, because what does it mean - clang-cl is the driver, but the option refers to the clang driver. The natural name would of course be -Xclang but that already means something else. Maybe we could just call it /clang:

Huh, I like that. You'd also be able to spell it -clang:, so the leading / is less of a disambiguator, but the colon is clearly a sign that this is a CL-style option.

I'm not completely convinced that we want this. So far we've used the strategy of promoting clang options that are also useful in clang-cl to core options, and if someone wants to use more clang than that, maybe clang-cl isn't the right driver for that use-case.

But I suppose an argument could be made for having an escape hatch from clang-cl if it doesn't add too much complexity to the code.

This is a good point. However, having this escape hatch gets you and Reid and others out of the business of having to promote options. Also, as new flags are added to the compiler people might need one revision of the official builds to realize they need a flag and one revision to get the flag added to the binary release. This obviously isn't a problem for people building Clang from source, but it does add unnecessary friction as I found myself.

I'm not sure I'm a fan of calling it /Xdriver: though, because what does it mean - clang-cl is the driver, but the option refers to the clang driver. The natural name would of course be -Xclang but that already means something else. Maybe we could just call it /clang:

At the conference last week I discussed this with Reid Kleckner. One of the options we discussed was trying to make things work such that -Xclang serves both purposes. We quickly decided that this wouldn't work. /clang: would be fine, but it might be more confusing since people will wonder what's the difference between /Xclang and /clang:. We could use something more verbose like /Xclang-driver:. I'd be happy to change the flag to whatever spelling will build consensus.

hans added a comment.Oct 23 2018, 5:02 AM

I'm not completely convinced that we want this. So far we've used the strategy of promoting clang options that are also useful in clang-cl to core options, and if someone wants to use more clang than that, maybe clang-cl isn't the right driver for that use-case.

But I suppose an argument could be made for having an escape hatch from clang-cl if it doesn't add too much complexity to the code.

This is a good point. However, having this escape hatch gets you and Reid and others out of the business of having to promote options. Also, as new flags are added to the compiler people might need one revision of the official builds to realize they need a flag and one revision to get the flag added to the binary release. This obviously isn't a problem for people building Clang from source, but it does add unnecessary friction as I found myself.

Yeah, let's add the escape hatch.

I'm not sure I'm a fan of calling it /Xdriver: though, because what does it mean - clang-cl is the driver, but the option refers to the clang driver. The natural name would of course be -Xclang but that already means something else. Maybe we could just call it /clang:

At the conference last week I discussed this with Reid Kleckner. One of the options we discussed was trying to make things work such that -Xclang serves both purposes. We quickly decided that this wouldn't work. /clang: would be fine, but it might be more confusing since people will wonder what's the difference between /Xclang and /clang:. We could use something more verbose like /Xclang-driver:. I'd be happy to change the flag to whatever spelling will build consensus.

Let's go with "/clang:" for the flag.

I don't think having both that and -Xclang would be too confusing. I think -Xclang is undocumented anyway and really something that should just be used by experts. We should add /clang: to the documentation and I think it will be straight-forward to understand.

Comment from the peanut gallery:

  • I like the whitelist model for gcc-style flags. It allows us to curate them (and /? output) since many don't make much sense in the cl world.
  • I like the idea behind this patch (and /clang: seems like a good spelling)
neerajksingh retitled this revision from clang-cl: Add "/Xdriver:" pass-through arg support. to clang-cl: Add "/clang:" pass-through arg support..
neerajksingh edited the summary of this revision. (Show Details)

Change the spelling of from "/Xdriver:" to "/clang:".

Refactor getIncludeExcludeOptionFlagMasks to take whether we're in CL mode as an argument, making the function pure.

hans added a comment.Oct 26 2018, 6:36 AM
One note about flag ordering: the /clang: flags are concatenated to the end of
the argument list, so in cases where the last flag wins, the /clang: flags
will be chosen regardless of their order relative to other flags on the driver
command line.

This seems a little unfortunate. I wonder if it would be possible to not have this restriction, i.e. to process these in-line with the rest of the flags?

One way to achieve this would be to change Driver::ParseArgStrings() to handle the "/clang:" arguments. After doing the regular option parsing, it would look for "/clang:" options and substitute them for the underlying option. This has the downside of adding some option-specific logic to ParseArgStrings, but on the other hand it's less intrusive than passing around the IsCLMode bool. Do you think something like this could work?

docs/UsersManual.rst
3111

Thanks for thinking about the docs!

I think we should put this above the /fallback section, since this new flag is more important and /fallback shouldn't be used much these days.

include/clang/Driver/CLCompatOptions.td
324

Do we really want the "OrSeparate" part of this? Is there any downside of limiting it to "/clang:-foo" rather than also allowing "/clang: -foo"?

One note about flag ordering: the /clang: flags are concatenated to the end of
the argument list, so in cases where the last flag wins, the /clang: flags
will be chosen regardless of their order relative to other flags on the driver
command line.

This seems a little unfortunate. I wonder if it would be possible to not have this restriction, i.e. to process these in-line with the rest of the flags?

One way to achieve this would be to change Driver::ParseArgStrings() to handle the "/clang:" arguments. After doing the regular option parsing, it would look for "/clang:" options and substitute them for the underlying option. This has the downside of adding some option-specific logic to ParseArgStrings, but on the other hand it's less intrusive than passing around the IsCLMode bool. Do you think something like this could work?

One difficulty occurs with options that have separated values, like the /clang:-MF /clang:<filename> case. In this case, we need to take two /clang: arguments and process them next to each other in order to form a single coherent argument/value pair. Another option is to allow the user to specify the option like /clang:"-MF <filename>" and then require that the user specify any flags that need a value in a single /clang: argument. This would require some code to split the value string on spaces before passing it on to clang argument parsing.

Do you have any preference or better suggestion for the option-with-value case?

Fix hans' comments.

neerajksingh marked 2 inline comments as done.Oct 26 2018, 5:49 PM
hans added a comment.Oct 29 2018, 3:11 AM
One note about flag ordering: the /clang: flags are concatenated to the end of
the argument list, so in cases where the last flag wins, the /clang: flags
will be chosen regardless of their order relative to other flags on the driver
command line.

This seems a little unfortunate. I wonder if it would be possible to not have this restriction, i.e. to process these in-line with the rest of the flags?

One way to achieve this would be to change Driver::ParseArgStrings() to handle the "/clang:" arguments. After doing the regular option parsing, it would look for "/clang:" options and substitute them for the underlying option. This has the downside of adding some option-specific logic to ParseArgStrings, but on the other hand it's less intrusive than passing around the IsCLMode bool. Do you think something like this could work?

One difficulty occurs with options that have separated values, like the /clang:-MF /clang:<filename> case. In this case, we need to take two /clang: arguments and process them next to each other in order to form a single coherent argument/value pair. Another option is to allow the user to specify the option like /clang:"-MF <filename>" and then require that the user specify any flags that need a value in a single /clang: argument. This would require some code to split the value string on spaces before passing it on to clang argument parsing.

Do you have any preference or better suggestion for the option-with-value case?

The -Xclang option has the same issue, and I think /clang: should work similarly, i.e. /clang:-MF /clang:<filename>. It's not pretty, but at least it's consistent. Yes, that means processing consecutive /Xclang: options together, but hopefully that's doable.

The -Xclang option has the same issue, and I think /clang: should work similarly, i.e. /clang:-MF /clang:<filename>. It's not pretty, but at least it's consistent. Yes, that means processing consecutive /Xclang: options together, but hopefully that's doable.

It looks like the handling for -Xclang is a lot simpler (in Clang::ConstructJob). There the Xclang options are all gathered and forwarded at a particular spot in the command line for cc1. They aren't interleaved with other options, and it wouldn't really make sense to do so anyway since it doesn't really look like cc1 arguments are constructed from driver arguments in any particular order.

The llvm/lib/Option/OptTable.cpp code is not really setup to easily interleave arguments like would be required for your request. Can you see a way to accomplish what you want without a deep refactoring of OptTable::ParseArgs and the InputArgList class?

hans added a comment.Oct 31 2018, 3:52 AM

The -Xclang option has the same issue, and I think /clang: should work similarly, i.e. /clang:-MF /clang:<filename>. It's not pretty, but at least it's consistent. Yes, that means processing consecutive /Xclang: options together, but hopefully that's doable.

It looks like the handling for -Xclang is a lot simpler (in Clang::ConstructJob). There the Xclang options are all gathered and forwarded at a particular spot in the command line for cc1. They aren't interleaved with other options, and it wouldn't really make sense to do so anyway since it doesn't really look like cc1 arguments are constructed from driver arguments in any particular order.

The llvm/lib/Option/OptTable.cpp code is not really setup to easily interleave arguments like would be required for your request. Can you see a way to accomplish what you want without a deep refactoring of OptTable::ParseArgs and the InputArgList class?

Okay, if it's hard to do, I suppose collecting the /clang: options and processing them separately after the others is the second-best option. We'll need to make sure the behaviour is well-documented.

dmajor added a subscriber: dmajor.Nov 1 2018, 5:36 AM

Make it clear in the documentation that the /clang flags are added to the end.

hans accepted this revision.Nov 6 2018, 5:31 AM

Okay, looks good to me (with a small nit).

docs/UsersManual.rst
2852

ultra nit: no space between the colon and <arg>

test/Driver/cl-options.c
624

I'm not sure this test adds much value..

This revision is now accepted and ready to land.Nov 6 2018, 5:31 AM
neerajksingh marked 2 inline comments as done.Nov 7 2018, 4:10 PM

Will update revision...

test/Driver/cl-options.c
624

This test ensures that the next test is actually testing something (i.e. that the settings change between the two runs).

neerajksingh marked an inline comment as done.

Reid, Hans, or someone else with commit access. If the revision looks good, could you please submit to SVN?

Any particular testing I should run beforehand? I ran the clang tests locally on Windows.

hans added a comment.Nov 8 2018, 3:17 AM

Reid, Hans, or someone else with commit access. If the revision looks good, could you please submit to SVN?

Any particular testing I should run beforehand? I ran the clang tests locally on Windows.

I'll run the tests on Linux and commit.

This revision was automatically updated to reflect the committed changes.

I noticed that this causes memory errors in certain situations. https://bugs.llvm.org/show_bug.cgi?id=42501 has details. Can you take a look?

Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 11:41 AM