This is an archive of the discontinued LLVM Phabricator instance.

Emit warnings from the driver for use of -mllvm or -Xclang options.
Needs RevisionPublic

Authored by jyknight on Nov 30 2018, 3:12 PM.

Details

Summary

This warning, Wexperimental-driver-option, is on by default, exempt
from -Werror, and may be disabled.

Additionally, change the documentation to note that these options are
not intended for common usage.

Event Timeline

jyknight created this revision.Nov 30 2018, 3:12 PM
chandlerc requested changes to this revision.Dec 4 2018, 1:45 AM

I think this should be internal-driver-option and the text updated? I don't think these are necessarily experimental, they're internals of the compiler implementation, and not a supported interface for users. Unsure how to best explain this.

This revision now requires changes to proceed.Dec 4 2018, 1:45 AM

I think this should be internal-driver-option and the text updated? I don't think these are necessarily experimental, they're internals of the compiler implementation, and not a supported interface for users. Unsure how to best explain this.

Yea, I was trying to figure out a good name. I thought "unsupported" at first, but that is a tricky word, since it has the connotation of "doesn't work in this version", which is not what I mean to imply -- the thing someone wants to do likely does work, but we need to indicate no guarantees about the future. Which is where "experimental" came from -- the options are useful for experimenting with compiler features that are not fully baked.

Perhaps "internal" is okay too...

Personally I'm against this type of warning as it's likely anyone using -mllvm is actually intending to adjust certain behaviors across one or more passes with a lot of switches supported by it being intentionally hidden from <llvm_tool> --help output requiring explicitly specifying that hidden flags be shown. One real use case being a dozen of patches to my downstream LLVM/Clang fork, for example *very* experimental support for SEH on Itanium ABI. I feel like this is going to affect developers more than users as now additional flags will have to be passed to suppress the warnings generated from using flags to debug/diagnose passes by code authors themselves. I feel like using -mllvm already implies explicit understanding of that, and of the cl::opt semantics/purpose as well as the flags being generally out of public view unless one gets them from full help (which explicitly shows all registered flags, hidden or not), or from source code which is most likely to be the developers themselves.

For example, I routinely use the following with SEH (excuse some of the naming, this is just a downstream fork however):

-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm -wtfabi-opts=0x1EF77F

I like the ability to pass those via the driver seamlessly, other options being explicitly constructing a clang -cc1 call myself, which is very verbose and the driver helps with that a huge amount or adding #if 0 around them downstream (better than commenting out since it's unlikely to cause merge conflicts).

I'm mostly indifferent towards -Xclang however (since I very rarely used it, I think -Xclang -fexternc-nounwind is the only time I used it, so I don't really have a strong opinion regarding that diagnostic, I still think it's not a good change. (speaking of, I should probably make a diff to expose that to the frontend via driver as it was seemingly missed in compiler invocation argument building, from its -f prefix I'm guessing that was accidental but I haven't looked into it, which I definitely should)).

If I may suggest another option, is it possible to add a "maintainer mode" flag to the build system (ie. with CMake, -DLLVM_MAINTAINER_MODE=ON, and a similar style thing with GN) and guard the diagnostic emission with #ifndef LLVM_MAINTAINER_MODE. This would easily allow developers to experiement with LLVM downstream without needing explicit workarounds for supressing those warnings. I would be happy to write a patch for CMake based builds myself (not GN unfortunately, slightly rusty on it) if you feel that is a better compromise. This means that downstream developers, whether intending to upstream such changes or not, can pass this through and not worry about those warnings since this is an explicit "here be dragons" opt-in, as that is easy to add to a build system (this also ties in with the previous discussion of adding an unsupressable warning for a certain -Xclang flag with the intent of getting users to avoid it in releases and yet allow performance data collection, but to developers that is more of an annoyance). I feel like this would be the ultimate consent to "yes I really want this, I'm aware of what it does" and would also require full rebuilds to enable, which is easy if you're developing a pass, for example, since you would be rebuilding anyway (assuming in good faith that this is only enabled for development builds, by downstream forks or build system configurations and releases never have it set). In case of CMake a warning may be a good idea as well when that flag is enabled as well as clear updates to documentation to reflect the purpose of it.

Anyway that's my opinion/concern on the matter, I don't know if others share it or not and I'm not sure if there are glaring problems with the idea of a solution I proposed, but I think it's better than some downstream vendors excluding this patch altogether and various other (inconsistent) ways developers will get around it.

Thanks.

Personally I'm against this type of warning as it's likely anyone using -mllvm is actually intending to adjust certain behaviors across one or more passes with a lot of switches supported by it being intentionally hidden from <llvm_tool> --help output requiring explicitly specifying that hidden flags be shown.

There is a cost to having people encode these flags into their build systems -- it can then cause issues if we ever change these internal flags. I do not think any Clang maintainer intends to support these as stable APIs, unlike most of the driver command-line. But, neither -Xclang nor -mllvm obviously scream out "don't use this!", and so people may then add them to their buildsystems without causing reviewers to say "Wait...really? Are you sure that's a good idea?".

That's why I think a warning is useful -- it'll discourage people from using them when they haven't properly understand the consequences, but does not prevent them from doing so, when they actually do.

For example, I routinely use the following with SEH (excuse some of the naming, this is just a downstream fork however):
-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm -wtfabi-opts=0x1EF77F

If you already are passing that, do you see a problem with instead passing
-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm -wtfabi-opts=0x1EF77F -Wno-experimental-driver-option
?

kristina added a comment.EditedDec 6 2018, 10:35 AM

There is a cost to having people encode these flags into their build systems -- it can then cause issues if we ever change these internal flags. I do not think any Clang maintainer intends to support these as stable APIs, unlike most of the driver command-line. But, neither -Xclang nor -mllvm obviously scream out "don't use this!", and so people may then add them to their buildsystems without causing reviewers to say "Wait...really? Are you sure that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled completely for people actively using those flags, which are pretty much exclusively toolchain developers (well basically what I proposed, since it's not clear what counts as a build made by someone who is working and debugging a pass, being fully aware of those flags, using the subset of them specific to that pass/feature, I would say assertion+dump enabled builds are closest, but having an explicit build flag for it would be nicer). It's more unified than everyone either adding workarounds into build systems or disabling it completely (by just removing it).

Alternatively just let people shoot themselves in the foot, a documentation change regarding the dangers of the flag should suffice. Besides, I don't think this really ever surfaced as an issue, until Chandler's idea with regards to an unsupressable warning for performance tests for 7.x.x (which is a very specific and narrow edge case to allow people to collect performance data). Outside of that very specific case, have we really had many issues with consumers accidentally setting weird flags that they would have to discover in the first place. I don't have a strong opinion on -Xclang but -mllvm <option> is verbose enough to use as is. Maybe it should be made a lot more obvious in one way or another but a warning by default seems like it's taking rather drastic preemptive measures against a non-issue (do correct me if I'm wrong here).

I do agree that the documentation should definitely scream that it's not stable and it's intended for maintainers since it's a convinient interface (the -mllvm one) for passing flags through the driver all the way to things like the MC layer, where needed, and yes these flags can be removed without notice, but again, I don't think it's our responsibility to protect users from using *intentionally* hidden flags aside from documenting the reason for why they're intentionally hidden in a more obvious way, I think the fact that they are intentionally not shown in standard LLVM help and yet are available contradicts the idea behind this patch, the whole purpose of the flag is to directly interact with specific internal parts of LLVM which in itself is not really intended to be a stable interface.

It's the reason the flags above in my example are -mllvm flags and not driver flags, because they're for a very experimental feature.

There is a cost to having people encode these flags into their build systems -- it can then cause issues if we ever change these internal flags. I do not think any Clang maintainer intends to support these as stable APIs, unlike most of the driver command-line. But, neither -Xclang nor -mllvm obviously scream out "don't use this!", and so people may then add them to their buildsystems without causing reviewers to say "Wait...really? Are you sure that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled completely for people actively using those flags, which are pretty much exclusively toolchain developers (well basically what I proposed, since it's not clear what counts as a build made by someone who is working and debugging a pass, being fully aware of those flags, using the subset of them specific to that pass/feature, I would say assertion+dump enabled builds are closest, but having an explicit build flag for it would be nicer). It's more unified than everyone either adding workarounds into build systems or disabling it completely (by just removing it).

I mean, I'm not much opposed to adding that -- just that any new build-time options is always a minor shame. But you didn't respond to the other part of my message -- is adding -Wno-experimental-driver-option to your compile-flags not already a completely sufficient solution for your use-case?

Besides, I don't think this really ever surfaced as an issue, until Chandler's idea with regards to an unsupressable warning for performance tests for 7.x.x

The primary impetus for me was actually the discovery that Boost's build system was using "-Xclang -include-pth" a few weeks back.

aaron.ballman added inline comments.
clang/docs/UsersManual.rst
3137

One downside to this is that I believe there are still situations where -Xclang may be required, such as when loading plugins. This could be mildly annoying even without it impacting -Werror because some CI tools do warning counts, it chats at users, etc.

thakis added a subscriber: thakis.Dec 6 2018, 10:50 AM

I don't have an opinion on this patch (if you force me to have one, I'm weakly in favor), but I agree with the general sentiment. When I told people to not use mllvm and Xclang before, they told me "but if I'm not supposed to use them, why are they advertised in --help"?

thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
  -Xclang <arg>           Pass <arg> to the clang compiler
thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
  -mllvm <value>          Additional arguments to forward to LLVM's option processing

Which to me is a valid point! Maybe we should remove them from --help or say "internal only, don't usually use" there?

arsenm added a subscriber: arsenm.Dec 6 2018, 10:52 AM

I don't really see the point of this and think it will just be an inconvenience to llvm developers.

Another use case we have for using these in a build system is for the builtin library shipped with the compiler

Oh hey, the patch does that already! Ignore me, then :-) That part is probably less controversial, maybe you want to land that while the warning part is being discussed?

I'm in favor of at least the documentation changes, though I'd like to see them use stronger language. I'm fairly in favor of the warning itself since its easy enough to disable (with the -Wno flag), so I don't terribly understand those against it.

george.karpenkov requested changes to this revision.Dec 6 2018, 10:59 AM
george.karpenkov added reviewers: dcoughlin, NoQ.
george.karpenkov added a subscriber: george.karpenkov.

Using -Xclang is the only way to pass options to the static analyzer, I don't think we should warn on it.

arphaman requested changes to this revision.Dec 6 2018, 11:08 AM

Swift uses -Xclang to pass in build settings to its own build and to pass in custom options through its Clang importer that we intentionally don't want to expose to Clang's users. We don't want to warn for those uses for sure.

I don't have an opinion on this patch (if you force me to have one, I'm weakly in favor), but I agree with the general sentiment. When I told people to not use mllvm and Xclang before, they told me "but if I'm not supposed to use them, why are they advertised in --help"?

thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
  -Xclang <arg>           Pass <arg> to the clang compiler
thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
  -mllvm <value>          Additional arguments to forward to LLVM's option processing

Which to me is a valid point! Maybe we should remove them from --help or say "internal only, don't usually use" there?

I think the documentation for -mllvm could definitely emphasize the aspect that these flags are not a part of any public or stable interface and are used to change internal behavior of LLVM components a lot more. Definitely in favor of doing that.

There is a cost to having people encode these flags into their build systems -- it can then cause issues if we ever change these internal flags. I do not think any Clang maintainer intends to support these as stable APIs, unlike most of the driver command-line. But, neither -Xclang nor -mllvm obviously scream out "don't use this!", and so people may then add them to their buildsystems without causing reviewers to say "Wait...really? Are you sure that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled completely for people actively using those flags, which are pretty much exclusively toolchain developers (well basically what I proposed, since it's not clear what counts as a build made by someone who is working and debugging a pass, being fully aware of those flags, using the subset of them specific to that pass/feature, I would say assertion+dump enabled builds are closest, but having an explicit build flag for it would be nicer). It's more unified than everyone either adding workarounds into build systems or disabling it completely (by just removing it).

I mean, I'm not much opposed to adding that -- just that any new build-time options is always a minor shame. But you didn't respond to the other part of my message -- is adding -Wno-experimental-driver-option to your compile-flags not already a completely sufficient solution for your use-case?

Besides, I don't think this really ever surfaced as an issue, until Chandler's idea with regards to an unsupressable warning for performance tests for 7.x.x

The primary impetus for me was actually the discovery that Boost's build system was using "-Xclang -include-pth" a few weeks back.

It would be an inconvenience to developers and a lot of patches grow from downstream, given that some people may want to disable the flag for development, it would basically mean that some may end up with varying workarounds as opposed to a uniform one. I don't think that happening would be of benefit to anyone. And yes I could add an extra flag for that configuration in the build system, in my case, but I don't think I'll be the only one doing that so again, this would just cause more fragmentation. I don't see that as a good thing.

Using -Xclang is the only way to pass options to the static analyzer, I don't think we should warn on it.

Well,, that seems unfortunate if we have the only supported interface for the static analyzer be an internal interface. Perhaps it can be given a different option? Even discounting this change, I that seems like it would be appropriate.

Swift uses -Xclang to pass in build settings to its own build and to pass in custom options through its Clang importer that we intentionally don't want to expose to Clang's users. We don't want to warn for those uses for sure.

I'm not sure if I understand correctly, so I'll try to reiterate: Swift calls clang internally, and when it does so, intentionally uses options that are not generally intended for others to use. Of course we shouldn't emit warnings in that case.

Is that a correct understanding? If so, doesn't it just make sense for that constructed command-line to disable the warning? And, isn't it good that it will warn, otherwise, in order to discourage people from using those flags that are intentionally-not-exposed?

I'm not sure that putting a warning that can be disabled really helps here; anyone who needs the option will just disable the warning anyway, and then users adding additional options somewhere else in the build system will miss the warning.

Instead, it would probably be better to rename Xclang and mllvm to something that makes it clear the user is doing something unsupported. Maybe "--unstable-llvm-option" and "--unstable-clang-option" or something like that. (This will lead to some breakage, but the breakage is roughly equivalent for anyone using -Werror.)

Well,, that seems unfortunate if we have the only supported interface for the static analyzer be an internal interface. Perhaps it can be given a different option? Even discounting this change, I that seems like it would be appropriate.

Officially there is, it's called -Xanalyzer, but in practice it does the same as -Xclang, and users use the two interchangeably.

Or actually, if you really want to discourage people from using them, maybe we could use the LLVM version number in the name, like "--unstable-llvm-option-8" (which would change to "--unstable-llvm-option-9" in in 9.0, etc.). This would allow developers to continue using the same workflows, but it would strongly discourage users from putting them into their build systems.

I don't think the argument that Swift or other users need Xclang options to hide them from users makes sense; stable workflows should use real driver flags. If the flag names are clear that they aren't intended for general use, that should be good enough.

kristina requested changes to this revision.EditedDec 6 2018, 11:56 AM

I'm not sure that putting a warning that can be disabled really helps here; anyone who needs the option will just disable the warning anyway, and then users adding additional options somewhere else in the build system will miss the warning.

Instead, it would probably be better to rename Xclang and mllvm to something that makes it clear the user is doing something unsupported. Maybe "--unstable-llvm-option" and "--unstable-clang-option" or something like that. (This will lead to some breakage, but the breakage is roughly equivalent for anyone using -Werror.)

Thinking about it more, downstream forks with custom passes may utilize those flags in tests, renaming them is definitely not the way to go, that is going to cause a lot of problems and possibly a lot of angry downstream users as well as contributors. Some out-of-tree test suites will treat warnings as failures so that behavior by default is also a possible cause for concern. I *really* think just changing the documentation to inform consumers about what the flags are intended for. In fact -mllvm is used extensively in a lot of lit/FileCheck tests, so that's also going to cause problems.

I think it's best to just document these options better, I agree, the documentation is extremely poor but anything beyond that will/could cause issues in so many places.

I'm not sure that putting a warning that can be disabled really helps here; anyone who needs the option will just disable the warning anyway, and then users adding additional options somewhere else in the build system will miss the warning.

Instead, it would probably be better to rename Xclang and mllvm to something that makes it clear the user is doing something unsupported. Maybe "--unstable-llvm-option" and "--unstable-clang-option" or something like that. (This will lead to some breakage, but the breakage is roughly equivalent for anyone using -Werror.)

Possibly you missed that this flag is exempt from -Werror -- it _only_ prints a warning, and is not an error, even when -Werror is specified.

NoQ added a comment.Dec 6 2018, 12:15 PM

Using -Xclang is the only way to pass options to the static analyzer, I don't think we should warn on it.

Well,, that seems unfortunate if we have the only supported interface for the static analyzer be an internal interface. Perhaps it can be given a different option? Even discounting this change, I that seems like it would be appropriate.

It's not really "supported" as in "we encourage users to use it". However, there's a third layer of "supported" here: we encourage external GUIs for the Static Analyzer to take advantage of these options.

Static Analyzer is, by design, almost unusable as a stand-alone command line tool and is only intended to be used via either the scan-build tool (a command-line tool that turns Static Analyzer's output into a fancy HTML output), or IDE intergration.

Different GUI developers will always want to use different internal flags and we cannot really control it. So, as a middle-ground solution, we keep these flags internal because people are not supposed to specify them manually (other than while developing the Static Analyzer itself), but GUIs are anyway allowed to take advantage of arbitrary combinations of them at their own risk.

It will be a good idea for us to settle at supporting different combinations, but we're not there yet. It might be a good idea to duplicate options that will be supported forever into frontend options, but this also needs work.

If the -Wwarn-drv-xclang-option is introduced, GUIs that use -Xclang and also display warnings will start displaying that warning, and will not stop doing it until they themselves are updated. This makes it harder to use new clang with old GUIs. Which is not a huge deal, but may have unexpected annoying consequences.

So, well, i'm not super against that, the overall idea seems good long-term, but i'm worried that there'd be a certain time interval of increased annoyance while the various GUI tools adapt.

In fact -mllvm is used extensively in a lot of lit/FileCheck tests, so that's also going to cause problems.

I count 13 uses of -mllvm in driver commands in-tree, and some of those are actually testing the flag itself. That doesn't seem like a lot. (This is excluding uses in CHECK lines and "clang -cc1" invocations.)

Possibly you missed that this flag is exempt from -Werror -- it _only_ prints a warning, and is not an error, even when -Werror is specified.

If the warning isn't going to honor -Werror, you might as well not print it at all; for many projects, nobody reads the logs of a successful build.