[clang] 2edb89c - Lex arguments for __has_cpp_attribute and friends as expanded tokens

[Aaron Ballman via cfe-commits]

On Mon, Oct 18, 2021 at 4:13 PM Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Mon, 18 Oct 2021 at 12:56, Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Mon, Oct 18, 2021 at 3:52 PM Richard Smith <richard at metafoo.co.uk> wrote:
>> >
>> >  On Mon, 18 Oct 2021 at 12:48, Aaron Ballman <aaron at aaronballman.com> wrote:
>> >>
>> >> On Mon, Oct 18, 2021 at 3:33 PM Richard Smith <richard at metafoo.co.uk> wrote:
>> >> >
>> >> > On Sun, 17 Oct 2021 at 04:58, Aaron Ballman via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>> >> >>
>> >> >>
>> >> >> Author: Aaron Ballman
>> >> >> Date: 2021-10-17T07:54:48-04:00
>> >> >> New Revision: 2edb89c746848c52964537268bf03e7906bf2542
>> >> >>
>> >> >> URL: https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542
>> >> >> DIFF: https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542.diff
>> >> >>
>> >> >> LOG: Lex arguments for __has_cpp_attribute and friends as expanded tokens
>> >> >>
>> >> >> The C and C++ standards require the argument to __has_cpp_attribute and
>> >> >> __has_c_attribute to be expanded ([cpp.cond]p5). It would make little sense
>> >> >> to expand the argument to those operators but not expand the argument to
>> >> >> __has_attribute and __has_declspec, so those were both also changed in this
>> >> >> patch.
>> >> >>
>> >> >> Note that it might make sense for the other builtins to also expand their
>> >> >> argument, but it wasn't as clear to me whether the behavior would be correct
>> >> >> there, and so they were left for a future revision.
>> >> >>
>> >> >> Added:
>> >> >>     clang/test/Preprocessor/has_attribute_errors.cpp
>> >> >>
>> >> >> Modified:
>> >> >>     clang/docs/ReleaseNotes.rst
>> >> >>     clang/lib/Lex/PPMacroExpansion.cpp
>> >> >>     clang/test/Preprocessor/has_attribute.c
>> >> >>     clang/test/Preprocessor/has_attribute.cpp
>> >> >>     clang/test/Preprocessor/has_c_attribute.c
>> >> >>
>> >> >> Removed:
>> >> >>
>> >> >>
>> >> >>
>> >> >> ################################################################################
>> >> >> diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
>> >> >> index 6501a4870e2a6..263eae83036df 100644
>> >> >> --- a/clang/docs/ReleaseNotes.rst
>> >> >> +++ b/clang/docs/ReleaseNotes.rst
>> >> >> @@ -110,6 +110,13 @@ Attribute Changes in Clang
>> >> >>    attribute is handled instead, e.g. in ``handleDeclAttribute``.
>> >> >>    (This was changed in order to better support attributes in code completion).
>> >> >>
>> >> >> +- __has_cpp_attribute, __has_c_attribute, __has_attribute, and __has_declspec
>> >> >> +  will now macro expand their argument. This causes a change in behavior for
>> >> >> +  code using ``__has_cpp_attribute(__clang__::attr)`` (and same for
>> >> >> +  ``__has_c_attribute``) where it would previously expand to ``0`` for all
>> >> >> +  attributes, but will now issue an error due to the expansion of the
>> >> >> +  predefined ``__clang__`` macro.
>> >> >> +
>> >> >>  Windows Support
>> >> >>  ---------------
>> >> >>
>> >> >>
>> >> >> diff  --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
>> >> >> index bf19f538647e6..5a0fa5184e38b 100644
>> >> >> --- a/clang/lib/Lex/PPMacroExpansion.cpp
>> >> >> +++ b/clang/lib/Lex/PPMacroExpansion.cpp
>> >> >> @@ -1293,7 +1293,7 @@ static bool EvaluateHasIncludeNext(Token &Tok,
>> >> >>  /// integer values.
>> >> >>  static void EvaluateFeatureLikeBuiltinMacro(llvm::raw_svector_ostream& OS,
>> >> >>                                              Token &Tok, IdentifierInfo *II,
>> >> >> -                                            Preprocessor &PP,
>> >> >> +                                            Preprocessor &PP, bool ExpandArgs,
>> >> >>                                              llvm::function_ref<
>> >> >>                                                int(Token &Tok,
>> >> >>                                                    bool &HasLexedNextTok)> Op) {
>> >> >> @@ -1319,7 +1319,10 @@ static void EvaluateFeatureLikeBuiltinMacro(llvm::raw_svector_ostream& OS,
>> >> >>    bool SuppressDiagnostic = false;
>> >> >>    while (true) {
>> >> >>      // Parse next token.
>> >> >> -    PP.LexUnexpandedToken(Tok);
>> >> >> +    if (ExpandArgs)
>> >> >> +      PP.Lex(Tok);
>> >> >> +    else
>> >> >> +      PP.LexUnexpandedToken(Tok);
>> >> >
>> >> >
>> >> > How does this handle things like:
>> >> >
>> >> > #define RPAREN )
>> >> > #if __has_attribute(clang::fallthrough RPAREN
>> >> >
>> >> > ? I think that should be an error: the ) token should not be produced by macro expansion, analogous to the behavior of function-like macros. But I imagine unless we're careful here, we'll allow that macro expansion to terminate the "macro".
>> >>
>> >> I agree, I think that should be an error. We handle it reasonably too; I get:
>> >>
>> >> error: missing ')' after 'clang'
>> >>
>> >> though it appears there is some compiler divergence here:
>> >> https://godbolt.org/z/c84cb3PW7
>> >
>> >
>> > Oops, I meant __has_cpp_attribute, not __has_attribute. We mishandle that one just like GCC: https://godbolt.org/z/Ej84Kca16
>
>
> Digging a bit more: https://godbolt.org/z/4Yo578b8n
>
> __has_cpp_attribute(clang::FT RPAREN and __has_attribute(FT RPAREN are incorrectly allowed, but __has_cpp_attribute(FT RPAREN is rejected.
>
>> Well that's just neat. I was trying to avoid lexing an unexpanded
>> token and then expanding it later if it was an identifier, but it
>> sounds like I should be taking that approach here. (Is that even
>> possible to do?)
>
>
> If you want to exactly match macro argument handling, you'll need to first lex unexpanded tokens until you find the `)`, and then expand them separately.

I investigated this approach, but it was inelegant to the extent that
I stopped investigating it. Some of the handlers do additional parsing
work, so replaying the tokens becomes a problem for those.

> Instead, you could ask if the ) token came from macro expansion, and if so, reject, but that will still mishandle cases like
>
> #define M1 M2(
> #define M2() fallthrough
> __has_cpp_attribute( M1 ) )

Agreed.

> Perhaps we can move the interception of builtin function-like macros to after we prepare to do macro expansion, so we can reuse the existing code for gathering the arguments?

That might be possible, but I'm not certain I have the time to explore
that right now.

I think the current patch is incremental progress in the right
direction (it allows us to macro expand the args) and the problems are
more with us being more permissive than we should be in ways users are
unlikely to hit by accident. Are you okay if I open a bug for the
additional issues, or do you think this should be reverted/addressed
more immediately?

~Aaron