[PATCH] Automatic detection of compatibility macros for non-portable diagnostics

This patch adds an automatic detection of a compatibility macros used in specific projects to hide constructions based on non-portable features (e.g. custom C++11 attributes). It helps to adapt diagnostic messages and fix-it hints to use these compatibility macros instead of the actual construct.

This feature is implemented in AnalysisBasedWarnings.cpp, as there’s currently only one diagnostic which gets profit from this - diagnostic of unannotated fall-through between switch labels. But the code of the CompatibilityMacroFinder class was intentionally made reasonably generic, and doesn’t contain any specific bindings to this diagnostic. The class is a lightweight handler of PPCallbacks::MacroDefined calls. An instance of it is registered via Preprocessor::addPPCallbacks for each token sequence (specified in plain text) to find in macros (currently only one). It keeps all macros with the specified expansion token sequence and then can determine which one can be used instead of the actual construct in a specific code location.

A motivating example for this feature:
There’s the -Wimplicit-fallthrough warning, which detects [[clang::fallthrough]]; construct as an annotation of an intended fall-through. In projects which should be compiled both in C++11 mode and in C++03 mode, this construct can not be used as is, so it should be wrapped in a macro, e.g.:

#ifdef clang

#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")

#define LLVM_FALLTHROUGH [[clang::fallthrough]]

#endif

#endif

#ifndef LLVM_FALLTHROUGH

#define LLVM_FALLTHROUGH do { } while (0)

#endif

Prior to this feature diagnostic messages would say:

test.cpp:156:5: warning: unannotated fall-through between switch labels
case 223:
^
test.cpp:156:5: note: insert ‘[[clang::fallthrough]];’ to silence this warning
case 223:
^
[[clang::fallthrough]];
test.cpp:156:5: note: insert ‘break;’ to avoid fall-through
case 223:
^
break;

Applying the first of these fix-it hints will lead to broken builds in C++03 mode, which is usually not desired.

But with the automatic detection feature the diagnostic will suggest the correct compatibility macro available in the corresponding source location:


test.cpp:156:5: note: insert ‘LLVM_FALLTHROUGH;’ to silence this warning

case 223:
^
LLVM_FALLTHROUGH;

Please, review this patch. Thank you!

fallthrough-compat-macro.diff (5.09 KB)

Wow! I would love to see this generalized.

I have had to use a couple of such macros in the past (notably for attribute(noreturn)) and it’s always annoying to have to grep through the headers to find where that compatibility macro had been registered.

– Matthieu

Hi Alex,

Please don’t add the PP callback if the fallthrough warning isn’t enabled. Also, have you made any performance measurements for this change?

I would prefer that the CompatibilityMacroFinder were given a list of token kinds and identifier infos to look for rather than a string. That would probably improve the performance a bit, and would allow your diagnostic to work in cases where the tokens don’t have the obvious spelling (for instance, “<:<:clang::fallthrough:>:>”).

FWIW, at a high level I find the callback approach is very worrisome.

The callback system was designed to be fast when the callbacks were completely null, and impose a non-trivial cost on the preprocessor otherwise.

Can we not walk back through the macro definitions after-the-fact? I would expect the preprocessor to still have all the information we need.

FWIW, at a high level I find the callback approach is very worrisome.

The callback system was designed to be fast when the callbacks were
completely null, and impose a non-trivial cost on the preprocessor
otherwise.

I do not think that we'll get measurably worse performance as a result of
this change. This code only works on macro definitions (does anyone know
how many of those per translation unit should we expect IRL?), and in most
cases this code will do no more than allocating a buffer (I can make this a
class member, so it doesn't get allocated each time) and comparing a
spelling of a couple of tokens (currently I wouldn't expect many macros
which expansion start with "[[").
I can also implement Richard's suggestions to make overhead even less: 1.
avoid adding callback when my warning isn't turned on; 2. lex the search
string so that matching is done by comparing tokens and not text. And I'm
going to run performance tests and find out if my patch really has some
measurable performance penalty. If it doesn't, do you have any objections
against callback approach?

Can we not walk back through the macro definitions after-the-fact? I would

expect the preprocessor to still have all the information we need.

I'm not sure that we retain contents of #undef'ed and redefined macros. And
we definitely need this as we run analysis after the whole file is lexed.

FWIW, at a high level I find the callback approach is very worrisome.

The callback system was designed to be fast when the callbacks were
completely null, and impose a non-trivial cost on the preprocessor
otherwise.

I do not think that we'll get measurably worse performance as a result of
this change. This code only works on macro definitions (does anyone know
how many of those per translation unit should we expect IRL?),

Boost provides libraries that do terrifying things here. I don't think we
can count on this number being "small".

Can we not walk back through the macro definitions after-the-fact? I would

expect the preprocessor to still have all the information we need.

I'm not sure that we retain contents of #undef'ed and redefined macros.
And we definitely need this as we run analysis after the whole file is
lexed.

But I would expect the macros we're particularly interested in
(compatibility ones) would not be undef'ed anywhere...

I do not think that we'll get measurably worse performance as a result of this change.

Please actually measure.

--Sean Silva

Yeah, that's the approach we take with NULL for example - we just
check if it's defined (& we're emitting these warnings potentially at
the end of the TU or function - so we don't get precision macro
state/could be wrong in particular cases of def/undef). Doesn't seem
unreasonable to me here there or here.

- David

On the Mac, Cocoa.h has 9000 macro definitions. Boost’s preprocessor library would be an interesting source of data here.

The preprocessor is extremely performance-sensitive. Allocating a buffer and performing string comparisons is not considered cheap. This will need measurement.

I object to the callback approach. If this is to be a core feature used by the preprocessor, parser, or semantic analysis to produce warnings, then it needs to be properly integrated into the AST, not placed alongside. The callbacks mechanism is for tools to extend Clang, not to decouple the implementation of Clang features.

I don’t know what you mean by “after the whole file is lexed”, since the lexer/preprocessor is integrated with the parser. There is certainly some skew when it comes to in-class member functions and template instantiations, which makes almost any approach sensitive to #undefs. Regardless, if a configuration macro is #undef’d at some point, then it’s perfectly reasonable to not provide a Fix-It that uses that macro.

It seems that what we need is a lightweight, generic way to detect whether a macro is a configuration macro, and then be able to query the set of configuration macros when we produce a warning/error where a Fix-It might use one of these configuration macros. I think it’s important to handle the generic case initially, because it’s going to affect performance significantly: the current patch has string comparisons for “[[clang::fallthrough]]”, but a generic solution would have many more string comparisons for a bunch of other attributes and forms.

  • Doug

I suppose that performance wise it may not be acceptable… but would it make sense to maintain a reverse mapping tokens => macro ?

We already have the macro => tokens mapping (and it survives even when Sema is running since it’s used by diagnosis), so if we could simply maintain another index alongside it then we would be “golden” would not we ?

(*) I realize that another index may not come cheap, however I would like to point out that Boost.MultiIndex for example manages to maintain multiple indexes without duplicating elements, so perhaps its approach could be reused (basically, it overlays multiple indexes on a single element).

– Matthieu

Well, it hinges on performance.

It does seem plausible to me that we could have the preprocessor keep a hold of macros after they have been #undef'd, so that we could look back in time and answer the question "which macros were defined when we were at the given source location." That would actually make a lot of sense for Clang, because it's the way the AST works (e.g., we keep all declarations of a function) and would actually benefit some features, such as code completion. But we'd have to carefully measure the performance and memory impact.

  - Doug

I thought a bit on the problem and now I’ll try to summarize the problem definition and the spectrum of possible solutions I see.

So, the problem is finding the most suitable spelling for a given code snippet. It can arise when formatting a diagnostic message, fix-it hint or performing a refactoring. In the first case it’s desired to avoid suggesting a spelling which can be wrong in the specific source location, in the last two cases it’s a must. So we need to be exact even when this means additional overhead.

“The most suitable spelling” can relate to different things and have different meaning (e.g. the least qualified type/function/variable name), but here it’s reasonable to limit the task to macros intended to hide implementation details for compatibility (e.g. with different compilers or language standards). I would also exclude the cases of function-like macros and macros defining only a part of a given snippet.

So the task is to find the last active macro expanding to a specified sequence of tokens. The “expanding to” part can mean “immediately expanding” or “recursively expanding”, and this alternative can have a significant effect on performance (the latter case would probably require using the TokenLexer or similar means for each comparison with the sample token sequence). I’m not sure if we need to support more complex case of finding recursively-expanded macros.

The structure of scopes of macro definitions is rather flat and can overlap the AST in an arbitrary way, so there’s no need to have an analog of AST for it or to include this information in AST. What we need is just a set of macros (with their expansion sequences) defined in any arbitrary source location. It could be indexed by contents or not, but the information is basically the same. The problem is that we can require this information for an earlier source location (as it happens in AnalysisBasedWarnings, that processes completely parsed functions, so the state of the preprocessor in the moment of processing can be different than in any given source location inside the function).

I see several approaches to this problem:

  1. before preprocessing define a set of token sequences we’re interested in; while preprocessing build a reverse mapping for token sequences of interest; look up the most suitable macro by iterating through a list of macros with a specific spelling;
  2. while preprocessing retain all #undef’ed macros with the respective source location information; look up macro by iterating over all macros and comparing their expansions with the sample;
  3. avoid any additional actions during preprocessing; when a look-up is requested re-run preprocessing from the beginning to the required source location and then iterate over all defined macros and find the latest defined one with the specific expansion sequence.

Variant 1 adds non-trivial steps to preprocessing phase, but allows faster look-ups. Variant 2 mostly adds a memory overhead, but shouldn’t add any computational overhead during preprocessing, but it will be slower for look-ups. Variant 3 only adds overhead during look-up, and it’s quite significant. If we want to optimize for a very few warnings case, variant 3 seems to be quite a good choice. Variant 2 may be a good trade-off in preprocessing and look-up overheads. Retaining of the #undef’d macros could possibly be useful for other purposes (not sure about this, but maybe someone has an example?). We could also come up with the variant 4, that would be a combination of 3 + 1: don’t do anything additional until we need the first look-up; once we need it, re-run preprocessor from the beginning and build a reverse mapping for the required sequence (or still make clients register all token sequences before preprocessing starts, and build a reverse mapping for all these sequences). This variant would add a significant overhead only for the first look-up, and all subsequent ones will come at a lower price. When there are no look-ups needed (i.e. compilation doesn’t trigger diagnostics that require spelling look-up), there will be absolutely no overhead both in memory and in processing.

–Best regards,
Alexander Kornienko

I suppose that performance wise it may not be acceptable… but would it make sense to maintain a reverse mapping tokens => macro ?

We already have the macro => tokens mapping (and it survives even when Sema is running since it’s used by diagnosis), so if we could simply maintain another index alongside it then we would be “golden” would not we ?

Well, it hinges on performance.

It does seem plausible to me that we could have the preprocessor keep a hold of macros after they have been #undef’d, so that we could look back in time and answer the question “which macros were defined when we were at the given source location.” That would actually make a lot of sense for Clang, because it’s the way the AST works (e.g., we keep all declarations of a function) and would actually benefit some features, such as code completion. But we’d have to carefully measure the performance and memory impact.

  • Doug

I thought a bit on the problem and now I’ll try to summarize the problem definition and the spectrum of possible solutions I see.

So, the problem is finding the most suitable spelling for a given code snippet. It can arise when formatting a diagnostic message, fix-it hint or performing a refactoring. In the first case it’s desired to avoid suggesting a spelling which can be wrong in the specific source location, in the last two cases it’s a must. So we need to be exact even when this means additional overhead.

“The most suitable spelling” can relate to different things and have different meaning (e.g. the least qualified type/function/variable name), but here it’s reasonable to limit the task to macros intended to hide implementation details for compatibility (e.g. with different compilers or language standards). I would also exclude the cases of function-like macros and macros defining only a part of a given snippet.

Sure. We could probably narrow this further to macros starting with attribute or [[.

So the task is to find the last active macro expanding to a specified sequence of tokens. The “expanding to” part can mean “immediately expanding” or “recursively expanding”, and this alternative can have a significant effect on performance (the latter case would probably require using the TokenLexer or similar means for each comparison with the sample token sequence). I’m not sure if we need to support more complex case of finding recursively-expanded macros.

I think we can safely ignore that case.

The structure of scopes of macro definitions is rather flat and can overlap the AST in an arbitrary way, so there’s no need to have an analog of AST for it or to include this information in AST. What we need is just a set of macros (with their expansion sequences) defined in any arbitrary source location. It could be indexed by contents or not, but the information is basically the same. The problem is that we can require this information for an earlier source location (as it happens in AnalysisBasedWarnings, that processes completely parsed functions, so the state of the preprocessor in the moment of processing can be different than in any given source location inside the function).

I see several approaches to this problem:

  1. before preprocessing define a set of token sequences we’re interested in; while preprocessing build a reverse mapping for token sequences of interest; look up the most suitable macro by iterating through a list of macros with a specific spelling;
  2. while preprocessing retain all #undef’ed macros with the respective source location information; look up macro by iterating over all macros and comparing their expansions with the sample;
  3. avoid any additional actions during preprocessing; when a look-up is requested re-run preprocessing from the beginning to the required source location and then iterate over all defined macros and find the latest defined one with the specific expansion sequence.

Variant 1 adds non-trivial steps to preprocessing phase, but allows faster look-ups. Variant 2 mostly adds a memory overhead, but shouldn’t add any computational overhead during preprocessing, but it will be slower for look-ups. Variant 3 only adds overhead during look-up, and it’s quite significant. If we want to optimize for a very few warnings case, variant 3 seems to be quite a good choice.

I’d rather not do variant 3. While it’s generally good to avoid paying for information for warnings that you don’t use, the cost here is going to be too high in the case where we do want to provide a warning.

Variant 2 may be a good trade-off in preprocessing and look-up overheads. Retaining of the #undef’d macros could possibly be useful for other purposes (not sure about this, but maybe someone has an example?). We could also come up with the variant 4, that would be a combination of 3 + 1: don’t do anything additional until we need the first look-up; once we need it, re-run preprocessor from the beginning and build a reverse mapping for the required sequence (or still make clients register all token sequences before preprocessing starts, and build a reverse mapping for all these sequences). This variant would add a significant overhead only for the first look-up, and all subsequent ones will come at a lower price. When there are no look-ups needed (i.e. compilation doesn’t trigger diagnostics that require spelling look-up), there will be absolutely no overhead both in memory and in processing.

As I noted before, the basis of variant 2 is interesting in its own right, because Clang should be retaining information about macros even after they are #undef’d (assuming this doesn’t have a too-high impact on memory usage).

Here’s a variant 5 that may strike the right balance: when we see a macro definition, look at just its first token or two to see whether it starts with attribute or [[. If so, mark it as potentially being a configuration macro. I suspect that this will have no measurable impact on performance.

When we do want to find a configuration macro for a given attribute, we look at the potential configuration macros and actually classify them, so that we can (from then on) efficiently answer the question “which configuration macro describes this particular attribute?”. This operation should require far less effort than variant 3, while still delaying most of the work.

Note that variant 5 can be implemented independently of whether we keep information about #undef’d macros in the preprocessor. It’s behavior will degrade gracefully, e.g., a configuration macro that is later #undef’d wouldn’t be found/suggested, but that’s not a case we should worry about. Keeping information about #undef’d macros would be orthogonal goodness.

  • Doug