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

Thu Dec 6 00:26:04 PST 2018

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