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


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

Reply via email to