[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D55150#1321793 , @jyknight wrote:

> In D55150#1321759 , 
> @george.karpenkov wrote:
>
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321829 , @efriedma wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina requested changes to this revision.
kristina added a comment.

In D55150#1321829 , @efriedma wrote:

> 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 problem 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321759 , @george.karpenkov 
wrote:

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

In D55150#1321771 , @arphaman wrote:

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In D55150#1321734 , @thakis wrote:

> 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
> -XclangPass  to the clang compiler
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
> -mllvm   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.

In D55150#1321724 , @jyknight wrote:

> In D55150#1321705 , @kristina wrote:
>
> > > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Alex Lorenz via Phabricator via cfe-commits]

arphaman requested changes to this revision.
arphaman added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added reviewers: dcoughlin, NoQ.
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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
-XclangPass  to the clang compiler
  thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
-mllvm   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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/UsersManual.rst:3137
   -W Enable the specified warning
-  -XclangPass  to the clang compiler
+  -XclangPass  to the clang cc1 frontend. For 
experimental use only.
 

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321705 , @kristina wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

> 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 ` 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321046 , @kristina wrote:

> 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 
> ` --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`
?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

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 
` --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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1318082 , @chandlerc wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55150/new/

https://reviews.llvm.org/D55150



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: rsmith, chandlerc.
Herald added a subscriber: arphaman.

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.


https://reviews.llvm.org/D55150

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3419,6 +3419,11 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setSuppressAfterFatalError(false);
 
+  // Suppress driver warning for use of -Xclang, since we add it ourselves.
+  Diags->setSeverityForGroup(diag::Flavor::WarningOrError,
+ "experimental-driver-option",
+ diag::Severity::Ignored, SourceLocation());
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4994,6 +4994,11 @@
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.
+  if (Args.hasArg(options::OPT_Xclang))
+D.Diag(diag::warn_drv_xclang_option);
+  if (Args.hasArg(options::OPT_mllvm))
+D.Diag(diag::warn_drv_mllvm_option);
+
   // -finclude-default-header flag is for preprocessor,
   // do not pass it to other cc1 commands when save-temps is enabled
   if (C.getDriver().isSaveTempsEnabled() &&
@@ -5926,6 +5931,8 @@
   CollectArgsForIntegratedAssembler(C, Args, CmdArgs,
 getToolChain().getDriver());
 
+  if (Args.hasArg(options::OPT_mllvm))
+D.Diag(diag::warn_drv_mllvm_option);
   Args.AddAllArgs(CmdArgs, options::OPT_mllvm);
 
   assert(Output.isFilename() && "Unexpected lipo output.");
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -466,7 +466,7 @@
   HelpText<"Pass  to the assembler">, MetaVarName<"">,
   Group;
 def Xclang : Separate<["-"], "Xclang">,
-  HelpText<"Pass  to the clang compiler">, MetaVarName<"">,
+  HelpText<"Pass  to the clang cc1 frontend. For experimental use only">, MetaVarName<"">,
   Flags<[DriverOption, CoreOption]>, Group;
 def Xcuda_fatbinary : Separate<["-"], "Xcuda-fatbinary">,
   HelpText<"Pass  to fatbinary invocation">, MetaVarName<"">;
@@ -2002,7 +2002,7 @@
 def mlinker_version_EQ : Joined<["-"], "mlinker-version=">,
   Flags<[DriverOption]>;
 def mllvm : Separate<["-"], "mllvm">, Flags<[CC1Option,CC1AsOption,CoreOption]>,
-  HelpText<"Additional arguments to forward to LLVM's option processing">;
+  HelpText<"Additional arguments to forward to LLVM's option processing. For experimental use only">;
 def mmacosx_version_min_EQ : Joined<["-"], "mmacosx-version-min=">,
   Group, HelpText<"Set Mac OS X deployment target">;
 def mmacos_version_min_EQ : Joined<["-"], "mmacos-version-min=">,
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1039,3 +1039,7 @@
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
+
+// Warning for command-line options that are not a supported part of the clang command-line interface.
+// Warnings in this group should be on by default, and exempt from -Werror.
+def ExperimentalDriverOption : DiagGroup<"experimental-driver-option">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -405,4 +405,14 @@
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;
+
+def warn_drv_xclang_option : Warning<
+  "-Xclang options are for experimental use only; future compatibility may not be preserved">,
+  InGroup, DefaultWarnNoWerror;
+
+def warn_drv_mllvm_option : Warning<
+  "-mllvm options are for experimental use only; future compatibility may not be preserved">,
+  InGroup, DefaultWarnNoWerror;
+
 }
+
Index: clang/docs/UsersManual.rst
===
---