[clang] 2edb89c - Lex arguments for __has_cpp_attribute and friends as expanded tokens
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 28 06:55:18 PDT 2021
On Mon, Oct 25, 2021 at 9:06 PM Nico Weber <thakis at chromium.org> wrote:
> Looks like the reason for this check was that
> https://github.com/llvm/llvm-project/blob/3b42fc8a07c37e47efae80c931eff7e63103e0e9/clang/include/clang/Basic/Attr.td#L1946
> contains __unsafe_unretained despite it just being a macro here:
> http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/InitPreprocessor.cpp#1019
>
> Is that Attr.td entry incorrect?
>
Aaron: Ping on ^ :)
>
> On Mon, Oct 25, 2021 at 8:40 PM Nico Weber <thakis at chromium.org> wrote:
>
>> Was this reviewed anywhere?
>>
>> I'll note that some internal project apparently used to check `#if
>> __has_attribute(__unsafe_unretained)`. That used to silently (and
>> incorrectly I think) always return 0 as far as I can tell, but now it fails
>> with:
>>
>> ```
>> $ out/gn/bin/clang -c foo.mm
>> foo.mm:1:21: error: missing ')' after '__attribute__'
>> #if __has_attribute(__unsafe_unretained)
>> ^~~~~~~~~~~~~~~~~~~
>> <built-in>:350:42: note: expanded from here
>> #define __unsafe_unretained __attribute__((objc_ownership(none)))
>> ~~~~~~~~~~~~~^
>> foo.mm:1:20: note: to match this '('
>> #if __has_attribute(__unsafe_unretained)
>> ^
>> 1 error generated.
>> ```
>>
>> That's arguably a progression and I'm still finding out what the intent
>> there was (maybe `#ifdef __unsafe_unretained`?).
>>
>> So nothing to do here I think, but I thought I'd mention it.
>>
>> On Sun, Oct 17, 2021 at 7:58 AM 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);
>>>
>>> already_lexed:
>>> switch (Tok.getKind()) {
>>> @@ -1609,21 +1612,21 @@ void Preprocessor::ExpandBuiltinMacro(Token
>>> &Tok) {
>>> OS << CounterValue++;
>>> Tok.setKind(tok::numeric_constant);
>>> } else if (II == Ident__has_feature) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> return II && HasFeature(*this, II->getName());
>>> });
>>> } else if (II == Ident__has_extension) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> return II && HasExtension(*this, II->getName());
>>> });
>>> } else if (II == Ident__has_builtin) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> @@ -1675,12 +1678,12 @@ void Preprocessor::ExpandBuiltinMacro(Token
>>> &Tok) {
>>> }
>>> });
>>> } else if (II == Ident__is_identifier) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [](Token &Tok, bool &HasLexedNextToken) -> int {
>>> return Tok.is(tok::identifier);
>>> });
>>> } else if (II == Ident__has_attribute) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, true,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> @@ -1688,7 +1691,7 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
>>> getTargetInfo(), getLangOpts()) : 0;
>>> });
>>> } else if (II == Ident__has_declspec) {
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, true,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> @@ -1704,8 +1707,8 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
>>> } else if (II == Ident__has_cpp_attribute ||
>>> II == Ident__has_c_attribute) {
>>> bool IsCXX = II == Ident__has_cpp_attribute;
>>> - EvaluateFeatureLikeBuiltinMacro(
>>> - OS, Tok, II, *this, [&](Token &Tok, bool &HasLexedNextToken) ->
>>> int {
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, true,
>>> + [&](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *ScopeII = nullptr;
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(
>>> Tok, *this, diag::err_feature_check_malformed);
>>> @@ -1719,7 +1722,8 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
>>> HasLexedNextToken = true;
>>> else {
>>> ScopeII = II;
>>> - LexUnexpandedToken(Tok);
>>> + // Lex an expanded token for the attribute name.
>>> + Lex(Tok);
>>> II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_feature_check_malformed);
>>> }
>>> @@ -1746,7 +1750,7 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
>>> Tok.setKind(tok::numeric_constant);
>>> } else if (II == Ident__has_warning) {
>>> // The argument should be a parenthesized string literal.
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> std::string WarningName;
>>> SourceLocation StrStartLoc = Tok.getLocation();
>>> @@ -1777,7 +1781,7 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
>>> // The argument to this builtin should be an identifier. The
>>> // builtin evaluates to 1 when that identifier names the module we
>>> are
>>> // currently building.
>>> - EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
>>> + EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
>>> [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,
>>>
>>> diag::err_expected_id_building_module);
>>> @@ -1837,28 +1841,32 @@ void Preprocessor::ExpandBuiltinMacro(Token
>>> &Tok) {
>>> return;
>>> } else if (II == Ident__is_target_arch) {
>>> EvaluateFeatureLikeBuiltinMacro(
>>> - OS, Tok, II, *this, [this](Token &Tok, bool &HasLexedNextToken)
>>> -> int {
>>> + OS, Tok, II, *this, false,
>>> + [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(
>>> Tok, *this, diag::err_feature_check_malformed);
>>> return II && isTargetArch(getTargetInfo(), II);
>>> });
>>> } else if (II == Ident__is_target_vendor) {
>>> EvaluateFeatureLikeBuiltinMacro(
>>> - OS, Tok, II, *this, [this](Token &Tok, bool &HasLexedNextToken)
>>> -> int {
>>> + OS, Tok, II, *this, false,
>>> + [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(
>>> Tok, *this, diag::err_feature_check_malformed);
>>> return II && isTargetVendor(getTargetInfo(), II);
>>> });
>>> } else if (II == Ident__is_target_os) {
>>> EvaluateFeatureLikeBuiltinMacro(
>>> - OS, Tok, II, *this, [this](Token &Tok, bool &HasLexedNextToken)
>>> -> int {
>>> + OS, Tok, II, *this, false,
>>> + [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(
>>> Tok, *this, diag::err_feature_check_malformed);
>>> return II && isTargetOS(getTargetInfo(), II);
>>> });
>>> } else if (II == Ident__is_target_environment) {
>>> EvaluateFeatureLikeBuiltinMacro(
>>> - OS, Tok, II, *this, [this](Token &Tok, bool &HasLexedNextToken)
>>> -> int {
>>> + OS, Tok, II, *this, false,
>>> + [this](Token &Tok, bool &HasLexedNextToken) -> int {
>>> IdentifierInfo *II = ExpectFeatureIdentifierInfo(
>>> Tok, *this, diag::err_feature_check_malformed);
>>> return II && isTargetEnvironment(getTargetInfo(), II);
>>>
>>> diff --git a/clang/test/Preprocessor/has_attribute.c
>>> b/clang/test/Preprocessor/has_attribute.c
>>> index 4970dc5904230..eef168e879103 100644
>>> --- a/clang/test/Preprocessor/has_attribute.c
>>> +++ b/clang/test/Preprocessor/has_attribute.c
>>> @@ -56,3 +56,11 @@ int has_no_volatile_attribute();
>>>
>>> #if __has_cpp_attribute(selectany) // expected-error {{function-like
>>> macro '__has_cpp_attribute' is not defined}}
>>> #endif
>>> +
>>> +// Test that macro expansion of the builtin argument works.
>>> +#define F fallthrough
>>> +
>>> +#if __has_attribute(F)
>>> +int has_fallthrough;
>>> +#endif
>>> +// CHECK: int has_fallthrough;
>>>
>>> diff --git a/clang/test/Preprocessor/has_attribute.cpp
>>> b/clang/test/Preprocessor/has_attribute.cpp
>>> index fe7d29f15de1a..bf0f9b3bc4a8f 100644
>>> --- a/clang/test/Preprocessor/has_attribute.cpp
>>> +++ b/clang/test/Preprocessor/has_attribute.cpp
>>> @@ -18,16 +18,6 @@ CXX11(clang::__fallthrough__)
>>> // CHECK: __gsl__::suppress: 0
>>> CXX11(__gsl__::suppress)
>>>
>>> -// We do somewhat support the __clang__ vendor namespace, but it is a
>>> -// predefined macro and thus we encourage users to use _Clang instead.
>>> -// Because of this, we do not support __has_cpp_attribute for that
>>> -// vendor namespace.
>>> -//
>>> -// Note, we can't use CXX11 here because it will expand __clang__ to 1
>>> -// too early.
>>> -// CHECK: 1::fallthrough: 0
>>> -__clang__::fallthrough: __has_cpp_attribute(__clang__::fallthrough)
>>> -
>>> // CHECK: _Clang::fallthrough: 201603L
>>> CXX11(_Clang::fallthrough)
>>>
>>> @@ -70,6 +60,50 @@ CXX11(unlikely)
>>> // CHECK: noreturn: 200809L
>>> // CHECK: unlikely: 201803L
>>>
>>> +namespace PR48462 {
>>> +// Test that macro expansion of the builtin argument works.
>>> +#define C clang
>>> +#define F fallthrough
>>> +#define CF clang::fallthrough
>>> +
>>> +#if __has_cpp_attribute(F)
>>> +int has_fallthrough;
>>> +#endif
>>> +// CHECK: int has_fallthrough;
>>> +
>>> +#if __has_cpp_attribute(C::F)
>>> +int has_clang_falthrough_1;
>>> +#endif
>>> +// CHECK: int has_clang_falthrough_1;
>>> +
>>> +#if __has_cpp_attribute(clang::F)
>>> +int has_clang_falthrough_2;
>>> +#endif
>>> +// CHECK: int has_clang_falthrough_2;
>>> +
>>> +#if __has_cpp_attribute(C::fallthrough)
>>> +int has_clang_falthrough_3;
>>> +#endif
>>> +// CHECK: int has_clang_falthrough_3;
>>> +
>>> +#if __has_cpp_attribute(CF)
>>> +int has_clang_falthrough_4;
>>> +#endif
>>> +// CHECK: int has_clang_falthrough_4;
>>> +
>>> +#define FUNCLIKE1(x) clang::x
>>> +#if __has_cpp_attribute(FUNCLIKE1(fallthrough))
>>> +int funclike_1;
>>> +#endif
>>> +// CHECK: int funclike_1;
>>> +
>>> +#define FUNCLIKE2(x) _Clang::x
>>> +#if __has_cpp_attribute(FUNCLIKE2(fallthrough))
>>> +int funclike_2;
>>> +#endif
>>> +// CHECK: int funclike_2;
>>> +}
>>> +
>>> // Test for Microsoft __declspec attributes
>>>
>>> #define DECLSPEC(x) x: __has_declspec_attribute(x)
>>> @@ -81,3 +115,13 @@ DECLSPEC(__uuid__)
>>>
>>> // CHECK: fallthrough: 0
>>> DECLSPEC(fallthrough)
>>> +
>>> +namespace PR48462 {
>>> +// Test that macro expansion of the builtin argument works.
>>> +#define U uuid
>>> +
>>> +#if __has_declspec_attribute(U)
>>> +int has_uuid;
>>> +#endif
>>> +// CHECK: int has_uuid;
>>> +}
>>>
>>> diff --git a/clang/test/Preprocessor/has_attribute_errors.cpp
>>> b/clang/test/Preprocessor/has_attribute_errors.cpp
>>> new file mode 100644
>>> index 0000000000000..1fc88d3f926fb
>>> --- /dev/null
>>> +++ b/clang/test/Preprocessor/has_attribute_errors.cpp
>>> @@ -0,0 +1,16 @@
>>> +// RUN: %clang_cc1 -triple i386-unknown-unknown -Eonly -verify %s
>>> +
>>> +// We warn users if they write an attribute like
>>> +// [[__clang__::fallthrough]] because __clang__ is a macro that expands
>>> to 1.
>>> +// Instead, we suggest users use [[_Clang::fallthrough]] in this
>>> situation.
>>> +// However, because __has_cpp_attribute (and __has_c_attribute) require
>>> +// expanding their argument tokens, __clang__ expands to 1 in the
>>> feature test
>>> +// macro as well. We don't currently give users a kind warning in this
>>> case,
>>> +// but we previously did not expand macros and so this would return 0.
>>> Now that
>>> +// we properly expand macros, users will now get an error about using
>>> incorrect
>>> +// syntax.
>>> +
>>> +__has_cpp_attribute(__clang__::fallthrough) // expected-error {{missing
>>> ')' after <numeric_constant>}} \
>>> + // expected-note {{to match
>>> this '('}} \
>>> + // expected-error {{builtin
>>> feature check macro requires a parenthesized identifier}}
>>> +
>>>
>>> diff --git a/clang/test/Preprocessor/has_c_attribute.c
>>> b/clang/test/Preprocessor/has_c_attribute.c
>>> index 670e42a97926e..36dd1c80e7802 100644
>>> --- a/clang/test/Preprocessor/has_c_attribute.c
>>> +++ b/clang/test/Preprocessor/has_c_attribute.c
>>> @@ -33,12 +33,45 @@ C2x(__gnu__::warn_unused_result)
>>> // CHECK: gnu::__warn_unused_result__: 201904L
>>> C2x(gnu::__warn_unused_result__)
>>>
>>> -// We do somewhat support the __clang__ vendor namespace, but it is a
>>> -// predefined macro and thus we encourage users to use _Clang instead.
>>> -// Because of this, we do not support __has_c_attribute for that
>>> -// vendor namespace.
>>> -//
>>> -// Note, we can't use C2x here because it will expand __clang__ to 1
>>> -// too early.
>>> -// CHECK: 1::fallthrough: 0
>>> -__clang__::fallthrough: __has_c_attribute(__clang__::fallthrough)
>>> +// Test that macro expansion of the builtin argument works.
>>> +#define C clang
>>> +#define L likely
>>> +#define CL clang::likely
>>> +#define N nodiscard
>>> +
>>> +#if __has_c_attribute(N)
>>> +int has_nodiscard;
>>> +#endif
>>> +// CHECK: int has_nodiscard;
>>> +
>>> +#if __has_c_attribute(C::L)
>>> +int has_clang_likely_1;
>>> +#endif
>>> +// CHECK: int has_clang_likely_1;
>>> +
>>> +#if __has_c_attribute(clang::L)
>>> +int has_clang_likely_2;
>>> +#endif
>>> +// CHECK: int has_clang_likely_2;
>>> +
>>> +#if __has_c_attribute(C::likely)
>>> +int has_clang_likely_3;
>>> +#endif
>>> +// CHECK: int has_clang_likely_3;
>>> +
>>> +#if __has_c_attribute(CL)
>>> +int has_clang_likely_4;
>>> +#endif
>>> +// CHECK: int has_clang_likely_4;
>>> +
>>> +#define FUNCLIKE1(x) clang::x
>>> +#if __has_c_attribute(FUNCLIKE1(likely))
>>> +int funclike_1;
>>> +#endif
>>> +// CHECK: int funclike_1;
>>> +
>>> +#define FUNCLIKE2(x) _Clang::x
>>> +#if __has_c_attribute(FUNCLIKE2(likely))
>>> +int funclike_2;
>>> +#endif
>>> +// CHECK: int funclike_2;
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211028/8deb6bb5/attachment-0001.html>
More information about the cfe-commits
mailing list