Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++03 mode ignores the arg in [[attribute(arg)]] #82995

Closed
AMP999 opened this issue Feb 26, 2024 · 5 comments · Fixed by #83065
Closed

C++03 mode ignores the arg in [[attribute(arg)]] #82995

AMP999 opened this issue Feb 26, 2024 · 5 comments · Fixed by #83065
Labels
c++ clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@AMP999
Copy link
Contributor

AMP999 commented Feb 26, 2024

https://godbolt.org/z/aPKq1sKPT
[[gnu::assume_aligned(4)]] static void *g() { return p; }
In -std=c++11 mode, this code is accepted and works as expected. In -std=c++03 mode, it seems to ignore the (4) argument for some reason, and I get an error:

<source>:5:3: error: 'assume_aligned' attribute takes at least 1 argument
    5 | [[gnu::assume_aligned(4)]] static void *g() { return p; }
      |   ^

@philnik777 I think you enabled attributes in 874217f; is there something missing to enable attributes with arguments?

The problem is these lines in ParseCXX11AttributeArgs.

  // If the attribute isn't known, we will not attempt to parse any
  // arguments.
  if (Form.getSyntax() != ParsedAttr::AS_Microsoft &&
      !hasAttribute(LO.CPlusPlus ? AttributeCommonInfo::Syntax::AS_CXX11
                                 : AttributeCommonInfo::Syntax::AS_C23,
                    ScopeName, AttrName, getTargetInfo(), getLangOpts())) {

This is reusing the return code of hasAttribute to ask whether the attribute is recognized (a boolean). But the machine-generated code in AttrHasAttributeImpl.inc uses the return code to say what __has_cpp_attribute should be set to (an int), and invariably returns 0 in C++03 mode. https://godbolt.org/z/casKPfdez

} else if (ScopeName == "gnu") {
  return llvm::StringSwitch<int>(Name)
[...]
    .Case("assume_aligned", LangOpts.CPlusPlus11 ? 1 : 0)

That file is generated by clang/utils/TableGen/ClangAttrEmitter.cpp. Maybe a change is needed in that generator?

@philnik777
Copy link
Contributor

I noticed that __has_cpp_attribute is broken a couple weeks ago myself, but I didn't realize that this breaks attribute parsing. I haven't had time to look at it closely yet, but the generator looks like a pretty good start. Do you want to work on that? If not, I'll try to fix this in the next few days, since it's more severe than I thought.

@philnik777
Copy link
Contributor

@AaronBallman do you think it's worth back-porting a fix for this?

@AaronBallman
Copy link
Collaborator

If the fix is small and obviously correct, then yes, I think that's worth backporting.

@EugeneZelenko EugeneZelenko added c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Feb 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 26, 2024

@llvm/issue-subscribers-clang-frontend

Author: Amirreza Ashouri (AMP999)

https://godbolt.org/z/aPKq1sKPT `[[gnu::assume_aligned(4)]] static void *g() { return p; }` In `-std=c++11` mode, this code is accepted and works as expected. In `-std=c++03` mode, it seems to ignore the `(4)` argument for some reason, and I get an error: ``` <source>:5:3: error: 'assume_aligned' attribute takes at least 1 argument 5 | [[gnu::assume_aligned(4)]] static void *g() { return p; } | ^ ``` @philnik777 I think you enabled attributes in 874217f; is there something missing to enable attributes with arguments?

The problem is these lines in ParseCXX11AttributeArgs.

  // If the attribute isn't known, we will not attempt to parse any
  // arguments.
  if (Form.getSyntax() != ParsedAttr::AS_Microsoft &amp;&amp;
      !hasAttribute(LO.CPlusPlus ? AttributeCommonInfo::Syntax::AS_CXX11
                                 : AttributeCommonInfo::Syntax::AS_C23,
                    ScopeName, AttrName, getTargetInfo(), getLangOpts())) {

This is reusing the return code of hasAttribute to ask whether the attribute is recognized (a boolean). But the machine-generated code in AttrHasAttributeImpl.inc uses the return code to say what __has_cpp_attribute should be set to (an int), and invariably returns 0 in C++03 mode. https://godbolt.org/z/casKPfdez

} else if (ScopeName == "gnu") {
  return llvm::StringSwitch&lt;int&gt;(Name)
[...]
    .Case("assume_aligned", LangOpts.CPlusPlus11 ? 1 : 0)

That file is generated by clang/utils/TableGen/ClangAttrEmitter.cpp. Maybe a change is needed in that generator?

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 26, 2024

@llvm/issue-subscribers-c-1

Author: Amirreza Ashouri (AMP999)

https://godbolt.org/z/aPKq1sKPT `[[gnu::assume_aligned(4)]] static void *g() { return p; }` In `-std=c++11` mode, this code is accepted and works as expected. In `-std=c++03` mode, it seems to ignore the `(4)` argument for some reason, and I get an error: ``` <source>:5:3: error: 'assume_aligned' attribute takes at least 1 argument 5 | [[gnu::assume_aligned(4)]] static void *g() { return p; } | ^ ``` @philnik777 I think you enabled attributes in 874217f; is there something missing to enable attributes with arguments?

The problem is these lines in ParseCXX11AttributeArgs.

  // If the attribute isn't known, we will not attempt to parse any
  // arguments.
  if (Form.getSyntax() != ParsedAttr::AS_Microsoft &amp;&amp;
      !hasAttribute(LO.CPlusPlus ? AttributeCommonInfo::Syntax::AS_CXX11
                                 : AttributeCommonInfo::Syntax::AS_C23,
                    ScopeName, AttrName, getTargetInfo(), getLangOpts())) {

This is reusing the return code of hasAttribute to ask whether the attribute is recognized (a boolean). But the machine-generated code in AttrHasAttributeImpl.inc uses the return code to say what __has_cpp_attribute should be set to (an int), and invariably returns 0 in C++03 mode. https://godbolt.org/z/casKPfdez

} else if (ScopeName == "gnu") {
  return llvm::StringSwitch&lt;int&gt;(Name)
[...]
    .Case("assume_aligned", LangOpts.CPlusPlus11 ? 1 : 0)

That file is generated by clang/utils/TableGen/ClangAttrEmitter.cpp. Maybe a change is needed in that generator?

philnik777 added a commit that referenced this issue Mar 1, 2024
…n C++03 (#83065)

The values for `__has_cpp_attribute` don't have to be guarded behind
`LangOpts.CPlusPlus` because `__has_cpp_attribute` isn't available if
Clang isn't in a C++ mode.

Fixes #82995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants