Pragma warning control

Hi,

I'm thinking of implementing a pragma to control Clang warnings. This
would look something like this (borrowing MS syntax because it makes
sense, IMO):

struct A;

void f(A *a) {
#pragma Clang warning(disable: warn_delete_incomplete)
  // This would cause the warning, but I know it to be safe:
  delete a;
  // Bring warning back to its default status so that real issues aren't
missed.
#pragma Clang warning(default: warn_delete_incomplete)
}

Alternatively, this could be done with an attribute, but I actually
think that a pragma is the better choice here. (Among other things,
because it can then control preprocessor, lexer and parser warnings.)

Is there interest in such a feature? I know that GCC's inability to
control most warnings within a source file causes problems in the Boost
project from time to time.

Some concrete issues I can think of:
- Template instantiation would ideally save the warning status around
the definition of a template and reintroduce it during instantiation;
else you for example disable truncation warnings around the template
definition, but instantiations cause the warning anyway.
- Explicitly disabling warnings at the command line could either share
status storage with this feature, but then warning(default: id) would
enable warnings that were disabled at the command line. Alternatively,
the feature could have its own storage, but that means doubling the
required memory.
- We would really need to make diagnostic continuations somehow a part
of the original diagnostic, or we could get dangling notes. (Or do we
get them already with -w? Do we have any warnings that have dependent
notes?)
- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

To implement this, I would create a lazily initialized map from names to
the actual integers. Better, from many viewpoints, would be a sorted
array of the names, searched by binary search (no initialization
necessary, it's all read-only data), but that's not possible due to the
lack of string processing abilities in the preprocessor and template
meta-engines. (In non-pompous terms, I can't sort strings at compile-time.)

Sebastian

Hi Sebastian,

I'm thinking of implementing a pragma to control Clang warnings. This
would look something like this (borrowing MS syntax because it makes
sense, IMO):

struct A;

void f(A *a) {
#pragma Clang warning(disable: warn_delete_incomplete)
// This would cause the warning, but I know it to be safe:
delete a;
// Bring warning back to its default status so that real issues aren't
missed.
#pragma Clang warning(default: warn_delete_incomplete)
}

Alternatively, this could be done with an attribute, but I actually
think that a pragma is the better choice here. (Among other things,
because it can then control preprocessor, lexer and parser warnings.)

I, too, prefer the pragma. Microsoft did a good job with this one. #pragma warning(push) and #pragma warning(pop) are particularly good for headers (where disabling warnings seems to matter the most to users).

Is there interest in such a feature?

Yes!

I know that GCC's inability to
control most warnings within a source file causes problems in the Boost
project from time to time.

Yes, it definitely annoys users when they can't turn off a bogus or unwanted warning.

Some concrete issues I can think of:
- Template instantiation would ideally save the warning status around
the definition of a template and reintroduce it during instantiation;
else you for example disable truncation warnings around the template
definition, but instantiations cause the warning anyway.

Yep. This probably means that these pragmas will need to be able to appear wherever (at least) declarations can appear.

- Explicitly disabling warnings at the command line could either share
status storage with this feature, but then warning(default: id) would
enable warnings that were disabled at the command line. Alternatively,
the feature could have its own storage, but that means doubling the
required memory.

I think warning(default: id) should not change its behavior based on the options on the command line.

- We would really need to make diagnostic continuations somehow a part
of the original diagnostic, or we could get dangling notes. (Or do we
get them already with -w? Do we have any warnings that have dependent
notes?)

There are other good reasons for doing this, too, such as providing non-terminal clients with enough information to group all of the notes as part of a single warning/error message.

- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

Yeah, and we'll want to try to weed out any duplicates, too (yuck). Still, this is work that we'll just have to do.

To implement this, I would create a lazily initialized map from names to
the actual integers. Better, from many viewpoints, would be a sorted
array of the names, searched by binary search (no initialization
necessary, it's all read-only data), but that's not possible due to the
lack of string processing abilities in the preprocessor and template
meta-engines. (In non-pompous terms, I can't sort strings at compile-time.)

We could require that the entries in DiagnosticKinds be sorted, with a Debug-only sortedness check at start-up. Actually, that would help force us to come up with more consistent diagnostic names, since getting related diagnostics together means giving them similar names.

I think this would make a great addition to Clang.

  - Doug

I think something like this would be better:

void f(A *a) {
  #pragma Clang warning(--warn_delete_incomplete)
   delete a;
  #pragma Clang warning(++warn_delete_incomplete)
}

So that nesting them still works, which I don't think the "default" way would.

Be nice for things like gcc's strict aliasing warnings, too. The
build could default to the stricter, false-positive including mode,
and it could be lowered to the only false negatives mode in sections
of the code that need it.

~ Scott

me22 wrote:

  

I'm thinking of implementing a pragma to control Clang warnings. This
would look something like this (borrowing MS syntax because it makes
sense, IMO):

struct A;

void f(A *a) {
#pragma Clang warning(disable: warn_delete_incomplete)
// This would cause the warning, but I know it to be safe:
delete a;
// Bring warning back to its default status so that real issues aren't
missed.
#pragma Clang warning(default: warn_delete_incomplete)
}

I think something like this would be better:

void f(A *a) {
  #pragma Clang warning(--warn_delete_incomplete)
   delete a;
  #pragma Clang warning(++warn_delete_incomplete)
}

So basically, you have a counter that is initialized to 0 if the warning
is disabled, and 1 if it's enabled, and the warning is emitted if its
counter is >= 1?
Interesting idea, but I think this would give users trying to reliably
disable warnings a headache - you'd find something like

// Just in case someone enabled the warning when it was already enabled:
#pragma Clang warning(--warn_delete_incomplete)
#pragma Clang warning(--warn_delete_incomplete)

I think supporting warning(push) and warning(pop) is the better
technique here. This allows proper nesting with headers.

Be nice for things like gcc's strict aliasing warnings, too. The
build could default to the stricter, false-positive including mode,
and it could be lowered to the only false negatives mode in sections
of the code that need it.
  

Aren't strict and weak mode different warnings? The Clang diagnostic
system certainly doesn't support different warning strictness levels.

Sebastian

Douglas Gregor wrote:

Hi Sebastian,

Some concrete issues I can think of:
- Template instantiation would ideally save the warning status around
the definition of a template and reintroduce it during instantiation;
else you for example disable truncation warnings around the template
definition, but instantiations cause the warning anyway.

Yep. This probably means that these pragmas will need to be able to
appear wherever (at least) declarations can appear.

What do you mean? In the code? A pragma can appear anywhere. (Although -
what *is* the effect of a pragma in the middle of an expression?)

- Explicitly disabling warnings at the command line could either share
status storage with this feature, but then warning(default: id) would
enable warnings that were disabled at the command line. Alternatively,
the feature could have its own storage, but that means doubling the
required memory.

I think warning(default: id) should not change its behavior based on
the options on the command line.

That's just a question of the definition of default's behavior. :slight_smile: Is
it defined to
- Enable the warning (but then it should be called 'enable')
- Enable the warning if it would be enabled without any pragma (then it
has to respect all command line options)
- Enable the warning if it would be enabled without any pragma or
command line option specific to this warning (then it has to respect
general options like -pedantic or -w).

The name 'default' in the MS compiler comes from their warning scheme.
The MS compiler has warning levels 0-4. Every warning has an associated
level and is enabled if the level is that or higher, so level 0 is no
warnings, and level 4 is all warnings. 'Default' then means to reset the
warning to the behavior it has at the specified warning level.
But Clang doesn't have warning levels, so we have to redefine or rename
default.

- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

Yeah, and we'll want to try to weed out any duplicates, too (yuck).
Still, this is work that we'll just have to do.

Yes. Another boring housekeeping job.

To implement this, I would create a lazily initialized map from names to
the actual integers. Better, from many viewpoints, would be a sorted
array of the names, searched by binary search (no initialization
necessary, it's all read-only data), but that's not possible due to the
lack of string processing abilities in the preprocessor and template
meta-engines. (In non-pompous terms, I can't sort strings at
compile-time.)

We could require that the entries in DiagnosticKinds be sorted, with a
Debug-only sortedness check at start-up. Actually, that would help
force us to come up with more consistent diagnostic names, since
getting related diagnostics together means giving them similar names.

That's a possibility, but it really requires a consistent naming scheme,
and one that is exposed to the user, so it has to make sense for both
developers and users.
So I think I'll go with building the map for now.

Sebastian

That's just a question of the definition of default's behavior. :slight_smile: Is
it defined to
- Enable the warning (but then it should be called 'enable')
- Enable the warning if it would be enabled without any pragma (then it
has to respect all command line options)
- Enable the warning if it would be enabled without any pragma or
command line option specific to this warning (then it has to respect
general options like -pedantic or -w).

The name 'default' in the MS compiler comes from their warning scheme.
The MS compiler has warning levels 0-4. Every warning has an associated
level and is enabled if the level is that or higher, so level 0 is no
warnings, and level 4 is all warnings. 'Default' then means to reset the
warning to the behavior it has at the specified warning level.
But Clang doesn't have warning levels, so we have to redefine or rename
default.

How about a stack-based approach? That is, a change is pushed on to the warning stack. A change is then reverted by popping it from the stack; either by naming it explicitly or an indiscriminate ‘pop’.

For example:

// warn about these
#pragma Clang warning(push: format-nonliteral)
… some code …
// revert this warning to its pre-specified behaviour
#pragma Clang warning(pop: format-nonliteral)

// don't warn about these
#pragma Clang warning(push: no-dead-store)
… some code …
// revert the most recent change
#pragma Clang warning(pop)

(An alternate syntax: ‘#pragma Clang push(warnings, no-dead-store)’, and so on.)

Maybe strict ordering of pushes and pops should be required; I'm mostly thinking of this as a user, so I have no opinion about that :slight_smile:

With regards to warning names, I'd suggest changing them to match command-line options. This involves removing the ‘warn’ prefix — which seems redundant to me — and changing underscores to dashes. Combined with a change to make diagnostic names more closely match GCC warning flags, and introducing ‘meta-diagnostics’ (if you can say that) to match things like -Wall, -Wextra, -Wformat, -Wsecurity and so on, affecting the default values of individual diagnostics. Prefixing a diagnostic name with ‘no-’ would have the effect of turning it off.

This might be useful for simplifying the driver, which would contain little or no information about the actual diagnostics. In the future, it might even be useful for retaining warning flags by embedding them into a serialised AST.

Having tried to compile a few programs with Clang, I noticed that warning options don't match GCC. It seems to me that naming core diagnostics compatibly would be preferable to having to translate them.

Is there interest in such a feature? I know that GCC's inability to
control most warnings within a source file causes problems in the Boost
project from time to time.

Yes, this would be a great feature! This is definitely worthwhile to work on. I also agree that pragmas are a better way to go than attributes.

I wouldn't worry too much about the template issues etc, we can solve some of that through extensions to the source location mechanism.

- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

I really think that this problem should be tackled first as a separate project. The "enum names" in DiagnosticKinds.def are intended to be unique identifiers, but are really not intended to be "stable".

In addition to warning control in the source code, we also need command-line option warning control: we already have some trivial options like -Wfloat-equal, -Wunused-macros, -Wundef, etc, but the current control over them is adhoc and controlled by C++ code in clang.cpp.

Going forward, we need to support more and more of these sorts of options in a "roughly" GCC-compatible way, but we also need to support harder things like -Wall and -W, which turn on a large set of different warnings.

I haven't thought too much about this, but I think it could make sense to introduce a notion of hierarchical groups of warning options. Each of these hierarchical groups could be assigned a "stable" name and could be controlled by both the command line and the #pragmas. A warning group would have its 1) stable name, 2) list of concrete diagnostics included, and 3) list of warning subgroups that it includes. This means that -Wall would just include a bunch of subgroups instead of hundreds of individual diagnostics.

This structure may be a bit overwhelming to handle with a .def file, if so, we could easily make a tblgen backend to handle this.

What do you think Sebastian? Do you agree that it would be good to tackle this problem before the #pragma version of it?

-Chris

Chris Lattner wrote:

- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

I really think that this problem should be tackled first as a separate
project. The "enum names" in DiagnosticKinds.def are intended to be
unique identifiers, but are really not intended to be "stable".

So, we define separate stable names?

In addition to warning control in the source code, we also need
command-line option warning control: we already have some trivial
options like -Wfloat-equal, -Wunused-macros, -Wundef, etc, but the
current control over them is adhoc and controlled by C++ code in
clang.cpp.

Going forward, we need to support more and more of these sorts of
options in a "roughly" GCC-compatible way, but we also need to support
harder things like -Wall and -W, which turn on a large set of
different warnings.

I haven't thought too much about this, but I think it could make sense
to introduce a notion of hierarchical groups of warning options. Each
of these hierarchical groups could be assigned a "stable" name and
could be controlled by both the command line and the #pragmas. A
warning group would have its 1) stable name, 2) list of concrete
diagnostics included, and 3) list of warning subgroups that it
includes. This means that -Wall would just include a bunch of
subgroups instead of hundreds of individual diagnostics.

This sounds like a good idea.

This structure may be a bit overwhelming to handle with a .def file,
if so, we could easily make a tblgen backend to handle this.

What do you think Sebastian? Do you agree that it would be good to
tackle this problem before the #pragma version of it?

Yes, we definitely need to tackle this first, since tackling it after
would mean rewriting the whole pragma stuff. I have no idea how tblgen
works, but I agree that the preprocessor is probably not powerful
enough. (Well, it is powerful enough, but the definition would be close
to unreadable.)

Sebastian

Chris Lattner wrote:

- We should probably clean up the names of the diagnostics. In the long
run, this means that the names of the diagnostics become part of the
public interface of the compiler, which means we can't change them at
will anymore.

I really think that this problem should be tackled first as a separate
project. The "enum names" in DiagnosticKinds.def are intended to be
unique identifiers, but are really not intended to be "stable".

So, we define separate stable names?

Yes, I think so. The warning options we already support directly control exactly one diagnostic, so we basically have (in tblgen syntax):

// Existing DiagnosticsKinds.def gets converted to tblgen syntax
def pp_macro_not_used : Warning<"macro is not used">;
def warn_floatingpoint_eq : Warning<"comparing floating point with == or != is unsafe">;
...

// New warning group definitions....
def unusedmacros : Group<"unused_macros", [pp_macro_not_used]>;
def floatequal : Group<"float-equal", [warn_floatingpoint_eq]>;

def readonlysetterattrs
    : Group<"readonly-setter-attrs", [warn_objc_property_attr_mutually_exclusive]>;

def formatnonliteral
    : Group<"format-nonliteral", [warn_printf_not_string_constant]>;
def undef : Group<[warn_pp_undef_identifier]>;

def implicitfunctiondeclaration
    : Group<"implicit-function-declaration", [warn_implicit_function_decl]>;

etc. However, going forward there will be other warnings that turn on several distinct warnings in the .def file. This is where having a list of warnings is useful.

After We have that, we can start aggregating them together, e.g.:

def all : Group<[undef, implicitfunctiondeclaration, ....]>;

Since tblgen can run arbitrary code, it can reject (or warn about) warnings that are not in a warning group etc. The code generated by tblgen could handle the -Wno-foo cases etc.

I'd suggest changing the existing command line option code for -W* in clang.cpp to just use a cl::list<std::string> WarningOpts("W", ...). Which will just make vector of all the -Wfoo options. The tblgen generated code can then just parse, validate and swizzle that array however it wants.

This structure may be a bit overwhelming to handle with a .def file,
if so, we could easily make a tblgen backend to handle this.

What do you think Sebastian? Do you agree that it would be good to
tackle this problem before the #pragma version of it?

Yes, we definitely need to tackle this first, since tackling it after
would mean rewriting the whole pragma stuff. I have no idea how tblgen
works, but I agree that the preprocessor is probably not powerful
enough. (Well, it is powerful enough, but the definition would be close
to unreadable.)

I can probably tackle this in a couple of weeks. If you're interested at poking at it, I'd suggest taking a look at llvm/include/llvm/Intrinsics.td. It gets tblgen'd into llvm/include/llvm/Intrinsics.gen. The code to do this is llvm/utils/TableGen/IntrinsicEmitter.*.

The first step would be to convert the existing diagnostickinds.def file to tblgen syntax and generate an Diagnostics.inc file that contains the existing views that we get with macro expansion.

-Chris

Douglas Gregor wrote:

Hi Sebastian,

Some concrete issues I can think of:
- Template instantiation would ideally save the warning status around
the definition of a template and reintroduce it during instantiation;
else you for example disable truncation warnings around the template
definition, but instantiations cause the warning anyway.

Yep. This probably means that these pragmas will need to be able to
appear wherever (at least) declarations can appear.

What do you mean? In the code? A pragma can appear anywhere. (Although -
what *is* the effect of a pragma in the middle of an expression?)

Yes, in the AST. We'd have some kind of PragmaWarningDecl/PragmaWarningStmt that can occur wherever a declaration or statement can occur, so that the instantiation of this warning/statement could perform the appropriate state changes. If we hit a pragma in the middle of an expression, we'd just put the pragma's decl/stmt before or after the current one and produce a warning to say what we did.

That said, Chris seems to have an idea about how to do this with SourceLocations, so I'd like to hear about that.

- Explicitly disabling warnings at the command line could either share
status storage with this feature, but then warning(default: id) would
enable warnings that were disabled at the command line. Alternatively,
the feature could have its own storage, but that means doubling the
required memory.

I think warning(default: id) should not change its behavior based on
the options on the command line.

That's just a question of the definition of default's behavior. :slight_smile: Is
it defined to
- Enable the warning (but then it should be called 'enable')

This is the behavior that I would prefer.

  - Doug

In my planned rewrite of source locations, SLoc's will get a hierarchical structure that can be walked. It would be reasonably easy to add a "was instantiated from" level for each time a token was instantiated in a template.

-Chris