proposal: every warning should have a -W flag

In feedback from developers using Clang, I have often heard requests such as "please put warning X under a flag" or, more generally, "please put every warning under a flag." I propose that we aim to achieve the latter goal.

I think there are several benefits of us doing this:

(1) Developers can cherry pick which warnings they want to suppress.

(2) Developers can cherry pick which warnings they want escalated to errors.

(3) -W flags provide a locus by which we can match compiler warnings with documentation. Some compiler warnings are more difficult to understand than others, and having an easy way to go from warning to a more thorough explanation would be beneficial to many users.

(4) Covering all warnings under -W flags helps us see what is all the warning behavior in the compiler.

Yesterday, I added a new tool in clang/tools called diagtool, whose goal is both to help analyze our use of diagnostics in clang but in the long term perhaps help in generating documentation for such warnings. Using diagtool, I discovered that only 48% of clang's warnings are currently controllable under a -W flag. While probably the most common warnings are under a flag, I've found that's often not the case (especially when a user files a bug requesting that a warning is placed under a flag). Moreover, I've also observed that as new warnings get added, they often lack -W flags. This can be particularly unfortunate for (legacy) code bases where such warnings pop up many times.

Here is my concrete proposal:

(1) We aim to gradually put all existing warnings under a -W flag. Multiple (related) warnings can be placed under the same -W flag (as they are often done now), but the main idea is to allow users to control these warnings.

(2) We require all new warnings to include a -W flag the moment they are added to Clang. We can enforce this by using diagtool in our tests (e.g., verify that the set of warnings without flags hasn't changed).

Comments? Concerns? Does everyone think this is a good idea, or are there arguments to the contrary?

Cheers,
Ted

(1) We aim to gradually put all existing warnings under a -W flag. Multiple (related) warnings can be placed under the same -W flag (as they are often done now), but the main idea is to allow users to control these warnings.

Is it worth considering whether they should all be under separate
flags (eg: VC numbers each diagnostic uniquely - the numbers are a bit
annoying, but it does make finding documentation/discussions on the
issues relatively easy & I can imagine we might end up with some
difficulty uniquely naming certain diagnostics)? I suppose I could see
the benefit of two technically distinct diagnostics being grouped
under the same name because they're different phrasings of the same
problem. At which point it's just a policy/semantic thing where we try
not to group things that aren't really tightly related - fuzzier to
keep track of, but possibly the right thing to do.

(2) We require all new warnings to include a -W flag the moment they are added to Clang. We can enforce this by using diagtool in our tests (e.g., verify that the set of warnings without flags hasn't changed).

I assume eventually this would be enforced syntactically (I don't know
enough about how the tblgen works to generate the diagnostic messages
- but would the documentation be able to be included alongside the
diagnostic message itself? I suppose that only works if it's a 1-1
mapping, otherwise you'd need some kind of grouping structure in the
tblgen itself such that no diagnostic could be outside a -W group.
That would also be awkward because each diagnostic would be in a
single place in the hierarchy while still possibly belonging to
multiple -W groups?)

- David

(1) We aim to gradually put all existing warnings under a -W flag. Multiple (related) warnings can be placed under the same -W flag (as they are often done now), but the main idea is to allow users to control these warnings.

Is it worth considering whether they should all be under separate
flags (eg: VC numbers each diagnostic uniquely - the numbers are a bit
annoying, but it does make finding documentation/discussions on the
issues relatively easy & I can imagine we might end up with some
difficulty uniquely naming certain diagnostics)? I suppose I could see
the benefit of two technically distinct diagnostics being grouped
under the same name because they're different phrasings of the same
problem. At which point it's just a policy/semantic thing where we try
not to group things that aren't really tightly related - fuzzier to
keep track of, but possibly the right thing to do.

I don't find it useful to number individual diagnostics, because it makes it harder to split a single diagnostic into several diagnostics further down the road. If someone has suppressed D1234, and we split that diagnostic into several, more-specific diagnostics, does suppressing D1234 still suppress those? Should it?

I'd much prefer that we have a logical grouping for our warnings, so we can document warning sets together.

(2) We require all new warnings to include a -W flag the moment they are added to Clang. We can enforce this by using diagtool in our tests (e.g., verify that the set of warnings without flags hasn't changed).

I assume eventually this would be enforced syntactically (I don't know
enough about how the tblgen works to generate the diagnostic messages
- but would the documentation be able to be included alongside the
diagnostic message itself? I suppose that only works if it's a 1-1
mapping, otherwise you'd need some kind of grouping structure in the
tblgen itself such that no diagnostic could be outside a -W group.
That would also be awkward because each diagnostic would be in a
single place in the hierarchy while still possibly belonging to
multiple -W groups?)

The documentation should go on the warning group itself.

  - Doug

I don't find it useful to number individual diagnostics, because it makes it harder to split a single diagnostic into several diagnostics further down the road. If someone has suppressed D1234, and we split that diagnostic into several, more-specific diagnostics, does suppressing D1234 still suppress those? Should it?

I'd much prefer that we have a logical grouping for our warnings, so we can document warning sets together.

We can have our cake and eat it too, perhaps. Group the warnings
logically, but allocate blocks of warning numbers with padding for
future expansion. For instance, numeric warnings are all in the D1000
- D1100 range. Then we can still split warnings into more specific
diagnostics, allocate them new numbers, but are able to keep the
grouping cohesive.

This, of course, is predicated on the thought that having individual
warning numbers is useful to the majority of people. From personal
experience, I like having a warning number because it makes it easier
to Google for others who've had the same issue. Most diagnostics
contain source-specific information, and so Google searches become a
guessing game of what keywords are important. YMMV

~Aaron

I don't find it useful to number individual diagnostics, because it makes it harder to split a single diagnostic into several diagnostics further down the road. If someone has suppressed D1234, and we split that diagnostic into several, more-specific diagnostics, does suppressing D1234 still suppress those? Should it?

I wasn't necessarily suggesting numbers themselves - we could stick
with names, but mostly my question was about "should we identify each
separate diagnostic" or is "every diagnostic must be part of at least
one group" the requirement we're aiming for? In the latter case we'll
just have to be more diligent about ensuring we get the right
granularity to make this work worthwhile (that is if we're trying to
address users coming to us & asking for suppression for a particular
warning it's not going to help to tell them "you can suppress that,
but you'll be suppressing a bunch of other things you like, too")

One way we could implement "every diagnostic has an identifier" while
still implementing your idea is simply to have the names of individual
diagnostics fit in with the names of sets: So initially we have a
"foo" warning then tomorrow we realize that foo is really only the
foo-bar case & we'd like to cover foo-baz too. So we rename foo to
foo-bar and promote foo-bar up to a set that includes foo-bar and
foo-baz.

I'd much prefer that we have a logical grouping for our warnings, so we can document warning sets together.

I think groupings make sense, too - but perhaps I don't understand the
details sufficiently. I see -Wall as a grouping, but I don't know if
-Wno-all is supported. If it is supported, then that's all we'd have -
from the broadest to the narrowest case - warning groups. Potentially
even groups of groups to make them easier to manage/document.

Then my question boils down: should we ensure that every diagnostic is
in a group of its own (in addition to any other groups) to ensure that
someone can disable any specific warning they're interested in. But I
do see your point: that two diagnostics aren't necessarily two
different warnings/problems (they might be different phrases for the
same problem) so I don't think it's terribly important that we make
standalone names/groups/sets for each diagnostic message. We just have
to make sure warning sets don't end up too large/vague.

(2) We require all new warnings to include a -W flag the moment they are added to Clang. We can enforce this by using diagtool in our tests (e.g., verify that the set of warnings without flags hasn't changed).

I assume eventually this would be enforced syntactically (I don't know
enough about how the tblgen works to generate the diagnostic messages
- but would the documentation be able to be included alongside the
diagnostic message itself? I suppose that only works if it's a 1-1
mapping, otherwise you'd need some kind of grouping structure in the
tblgen itself such that no diagnostic could be outside a -W group.
That would also be awkward because each diagnostic would be in a
single place in the hierarchy while still possibly belonging to
multiple -W groups?)

The documentation should go on the warning group itself.

Thinking about my original statement I realized we could potentially
invert the structure, basically: Each diagnostic must declare itself
to be part of one or more warning groups. A warning group is then just
a name and documentation & any warnings can insert themselves into it.
That way we can easily ensure all warnings are part of a group.

- David

We can have our cake and eat it too, perhaps. Group the warnings
logically, but allocate blocks of warning numbers with padding for
future expansion. For instance, numeric warnings are all in the D1000
- D1100 range. Then we can still split warnings into more specific
diagnostics, allocate them new numbers, but are able to keep the
grouping cohesive.

To be honest I wasn't really pushing the numbering aspect of VC's
diagnostic handling so much as I was the unique-ness.

This, of course, is predicated on the thought that having individual
warning numbers is useful to the majority of people. From personal
experience, I like having a warning number because it makes it easier
to Google for others who've had the same issue. Most diagnostics
contain source-specific information, and so Google searches become a
guessing game of what keywords are important. YMMV

But yes, this is the selling point for having numbered diagnostics. I
do actually prefer the named diagnostics that clang uses in both
compiler output and pragma suppressions - they're more
self-documenting (when I look through a make file & see suppressions
for 3 different VC warnings as just numbers - they either have to have
comments or I have to google up the docs to see what they do. With
clang I don't get random numbers in my compiler messages, instead I
get a nice string telling me the name of the warning so I can suppress
it if I want (or turn it on if I'm not seeing that in another build)).

& the names clang uses should be fairly searchable (obviously as clang
usage increases & these sort of questions turn up in documentation,
forums, etc, more)

- David

We can have our cake and eat it too, perhaps. Group the warnings
logically, but allocate blocks of warning numbers with padding for
future expansion. For instance, numeric warnings are all in the D1000
- D1100 range. Then we can still split warnings into more specific
diagnostics, allocate them new numbers, but are able to keep the
grouping cohesive.

To be honest I wasn't really pushing the numbering aspect of VC's
diagnostic handling so much as I was the unique-ness.

This, of course, is predicated on the thought that having individual
warning numbers is useful to the majority of people. From personal
experience, I like having a warning number because it makes it easier
to Google for others who've had the same issue. Most diagnostics
contain source-specific information, and so Google searches become a
guessing game of what keywords are important. YMMV

But yes, this is the selling point for having numbered diagnostics.

This is the selling point of having a unique searchable name. It doesn't have to be an otherwise-meaningless number.

I
do actually prefer the named diagnostics that clang uses in both
compiler output and pragma suppressions - they're more
self-documenting (when I look through a make file & see suppressions
for 3 different VC warnings as just numbers - they either have to have
comments or I have to google up the docs to see what they do. With
clang I don't get random numbers in my compiler messages, instead I
get a nice string telling me the name of the warning so I can suppress
it if I want (or turn it on if I'm not seeing that in another build)).

& the names clang uses should be fairly searchable (obviously as clang
usage increases & these sort of questions turn up in documentation,
forums, etc, more)

Yeah, agreed on all points.

  - Doug

This, of course, is predicated on the thought that having individual
warning numbers is useful to the majority of people. From personal
experience, I like having a warning number because it makes it easier
to Google for others who've had the same issue. Most diagnostics
contain source-specific information, and so Google searches become a
guessing game of what keywords are important. YMMV

But yes, this is the selling point for having numbered diagnostics.

This is the selling point of having a unique searchable name. It doesn't have to be an otherwise-meaningless number.

Definitely agreed. I think the hard part is the uniqueness of the
names. You don't run into that with meaningless numbers. But as was
pointed out, suppressing random numbers in code isn't exactly the best
thing ever either. So in that regard, unique names are definitely
preferred.

I
do actually prefer the named diagnostics that clang uses in both
compiler output and pragma suppressions - they're more
self-documenting (when I look through a make file & see suppressions
for 3 different VC warnings as just numbers - they either have to have
comments or I have to google up the docs to see what they do. With
clang I don't get random numbers in my compiler messages, instead I
get a nice string telling me the name of the warning so I can suppress
it if I want (or turn it on if I'm not seeing that in another build)).

& the names clang uses should be fairly searchable (obviously as clang
usage increases & these sort of questions turn up in documentation,
forums, etc, more)

Yeah, agreed on all points.

I'm sold. :slight_smile:

~Aaron

I was specifically proposing that…

(a) every warning is part of a group

and

(b) every group that contains a warning that can only be activated by that group maps to a -W flag

All of our diagnostics have unique tablegen entries, so getting unique names for -W flags might not be that hard. When one looks at the .td files, many of the diagnostic names directly correspond to the -W flag.

All of our diagnostics have unique tablegen entries, so getting unique names
for -W flags might not be that hard. When one looks at the .td files, many
of the diagnostic names directly correspond to the -W flag.

Oh, of course. So would it be worth just making every diagnostic its
own flag by definition, using the tablegen name for the -W flag?

I was specifically proposing that…
(a) every warning is part of a group
and
(b) every group that contains a warning that can only be activated by that group maps to a -W flag

Does LLVM have the concept of groups that don't have an associated -W
flag? what are they for?

- David

All of our diagnostics have unique tablegen entries, so getting unique names
for -W flags might not be that hard. When one looks at the .td files, many
of the diagnostic names directly correspond to the -W flag.

Oh, of course. So would it be worth just making every diagnostic its
own flag by definition, using the tablegen name for the -W flag?

I don't think so, or at least I wouldn't make it automatic. Some diagnostics are just variants of basically the same thing, or logically part of a single warning. For example, consider the different format string diagnostics:

def warn_printf_insufficient_data_args : Warning<
  "more '%%' conversions than data arguments">, InGroup<Format>;
def warn_format_invalid_conversion : Warning<
  "invalid conversion specifier '%0'">, InGroup<Format>;

and so on.

Is there real value in placing each of these under a separate flag? In the general case, I don't think so, but where it makes sense it seems reasonable to break them down into separate -W flags (which we do in some cases for format string warnings).

I was specifically proposing that…
(a) every warning is part of a group
and
(b) every group that contains a warning that can only be activated by that group maps to a -W flag

Does LLVM have the concept of groups that don't have an associated -W
flag? what are they for?

That's a good point. I don't think one can define a warning group without an associated flag. Groups, however, can contain other groups, so when we report warnings we include the flag with the most precise coverage.

Some diagnostics are just variants of basically the same thing, or logically part of a single warning. For example, consider the different format string diagnostics:

def warn_printf_insufficient_data_args : Warning<
"more '%%' conversions than data arguments">, InGroup<Format>;
def warn_format_invalid_conversion : Warning<
"invalid conversion specifier '%0'">, InGroup<Format>;

and so on.

Is there real value in placing each of these under a separate flag? In the general case, I don't think so, but where it makes sense it seems reasonable to break them down into separate -W flags (which we do in some cases for format string warnings).

Yep - as much as I like simple mechanical rules for correctness, I was
standing to lean in that direction too. Thanks for the clear answer &
example.

Does LLVM have the concept of groups that don't have an associated -W
flag? what are they for?

That's a good point. I don't think one can define a warning group without an associated flag. Groups, however, can contain other groups, so when we report warnings we include the flag with the most precise coverage.

How does LLVM do that currently? It searches through all the groups
that contain a diagnostic & finds the group with the smallest size?

Would it make sense (as a means to enforce/define correctness of this
new warning invariant, and possibly make it easier to manage warning
groups) to define a primary group for each warning. In that way no
warning could be without a group, there would be no need to
calculate/discover the appropriate group to flag for this warning, and
you couldn't produce mismatched warning groups (eg: if
warn_printf_insufficient_data_args and warn_format_invalid_conversion
had a primary group of invalid-format, then they could never appear in
two different other warning groups ('extra' couldn't have
warn_printf_insufficient_data_args without
warn_format_invalid_conversion - 'extra' would simply contain
'invalid-format'))

Or is that just not worth worrying about & we'll use a test case or
build step to validate the tablegen warning list indefinitely?

Also - if we did this, we could institute it almost immediately by
using the current warning names as a first pass, even if they're not
the best names/don't account for logical groupings we'd like. That way
there would be a name for every warning sooner, just not the best name
- and we could fix that up as needed/desired along the way. The
drawback being that the poorly grouped/named warnings wouldn't be as
obvious & might confuse new developers into thinking that was the
norm/appropriate way to do things (but CR should handle that).

- David

Yes, please. I would also like to see semantic compatibility with GCC as
much as possible, as in the same suspicious behavior can be checked for
with the same flag or the check disabled.

I fully agree with the rest of your proposal.

Joerg

I love this. It’s been unwritten policy for the warnings we’re adding, and I think it’d be good to do it generally.

I’ll particularly like that we can test and ensure that new warnings are always given a flag. =] Hopefully we can help move some of the warnings under flags that aren’t yet.

Regarding the unique-ID-per-warning discussion (which was a bit of a tangent), I think more data in the already busy warning messages would be unfortunate. As we already print the flag name in most cases, I’d rather see documentation use the flag name as a searchable bucket, and then a list of example warning text and code triggering it so that people can quickly find the warning they’re hitting. Then we might provide facilities to re-generate the documentation by re-running Clang over the examples, thus easing the process of keeping the documentation up to date with the latest spelling of the message.

Joerg Sonnenberger
<joerg@britannica.bec.de> writes:

Yes, please. I would also like to see semantic compatibility with GCC as
much as possible, as in the same suspicious behavior can be checked for
with the same flag or the check disabled.

Yes please!

Clang's gcc-compatibility [both for source and invocation] is absolutely
brilliant for users, and it's done a great job of it so far. But of
course for every new feature added, there's potential for divergence,
and it would be nice to at keep that in check as much as possible...

Is there some established mechanism for coordinating interface changes
with gcc where it would make sense?

Thanks,

-Miles

For existing warnings, we try and match GCC’s flag names. For new warnings being added to Clang that (to our knowledge) aren’t being added to GCC, there is no dialog with the GCC developers to support the same warning flag. I think dialog between the communities would be helpful in this regard, but I’m not interested in establishing an official process here for coordinating warnings between the two compilers. Generally speaking it hasn’t been a problem at all, and that formal process would likely burden both communities.

Okay, the consensus is we should do this. We can haggle out the naming of -W flags we go forward.

I’ve added the diagtool test that enforces that all new warnings require a flag:

http://llvm.org/viewvc/llvm-project?view=rev&revision=137369

hi kremenek,
Thinking one step forward, For the new warnings not existed now, if GCC added the same warning flag later, could Clang support that flag as well?
I’m asking that because we need use both compilers, if the new options are different, it might break the current command line compatibility.
Does this make sense?

Sure, there’s nothing preventing us from adding aliases for warnings (or, in rare cases, changing warning flags), although ideally GCC will hopefully try and match our choice of warning flags as well when they add new ones. :slight_smile: