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

Wed Aug 1 10:28:00 PDT 2012

On Tue, Jul 31, 2012 at 8:47 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jul 27, 2012, at 4:07 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
>
> On Fri, Jul 27, 2012 at 11:40 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> 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?),
>
>
> On the Mac, Cocoa.h has 9000 macro definitions. Boost's preprocessor
> library would be an interesting source of data here.
>
> 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 "[[").
>
>
> The preprocessor is extremely performance-sensitive. Allocating a buffer
> and performing string comparisons is not considered cheap. This will need
> measurement.
>
> 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?
>
>
> 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.
>
> 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.
>
>
> 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
>
>
>
>  On Fri, Jul 27, 2012 at 2:36 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> 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:>:>").
>>>
>>>
>>> On Tue, Jul 24, 2012 at 10:11 AM, Alexander Kornienko <alexfh at google.com
>>> > wrote:
>>>
>>>> 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!
>>>>
>>>> --
>>>> Best regards,
>>>> Alexander Kornienko
>>>>
>>>
> --
> Best regards,
> Alexander Kornienko
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120801/556135c4/attachment.html>