Bug 110734 - Attributes cannot be applied to asm declaration
Summary: Attributes cannot be applied to asm declaration
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 13.2.0
: P3 enhancement
Target Milestone: 14.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, inline-asm
Depends on:
Blocks:
 
Reported: 2023-07-19 07:45 UTC by Julian Waters
Modified: 2023-12-05 17:04 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-07-19 00:00:00


Attachments
Attribute for asm declarations (2.61 KB, patch)
2023-11-29 10:45 UTC, Julian Waters
Details | Diff
Attribute for asm declarations (2.71 KB, patch)
2023-11-30 14:59 UTC, Julian Waters
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Waters 2023-07-19 07:45:59 UTC
Consider the following:

[[gnu::no_reorder]]
asm ("nop");

The correct gcc warning should be that "attributes in front of statements
are ignored", signifying that the asm was correctly processed as a
statement, and the attribute dropped during that processing, but instead:

error.cpp:51:5: error: expected primary-expression before 'asm'
   51 |     asm ("nop" "\n\t"
      |     ^~~
error.cpp:50:5: warning: attributes at the beginning of statement are
ignored [-Wattributes]
   50 |     [[gnu::no_reorder]]
      |     ^~~~~~~~~~~~~~~~~~~

The compiler errors out, with the parser strangely having expected an
expression after the attribute. Afterwards, it then confusingly parses the
asm statement and discards the attribute correctly, so I am fairly certain
this is a bug. The attribute above may not be a very good example, but
there are attributes like gnu::hot and gnu::cold which are supposed to work
with asm statements.
Comment 1 Julian Waters 2023-07-19 07:47:34 UTC
Set version to 13.1, but this problem is very likely in every version
Comment 2 Julian Waters 2023-07-19 07:57:22 UTC
Is this accurately described as an enhancement? This is erroneous behaviour with something that is already meant to be a feature in gcc (applying attributes to statements, as seen inside parser.cc), not a feature request for new behaviour
Comment 3 Richard Biener 2023-07-19 09:43:19 UTC
But a toplevel asm isn't a "statement" in the sense of parsing.  Still confirmed.  Note that I'm not sure we can handle no-reorder for toplevel
asms, IIRC they are not entities in the symtab.  I don't remember off head
where they are recorded either ...

Btw, I don't get

t.c:3:1: warning: attributes at the beginning of statement are ignored [-Wattributes]
    3 | [[gnu::no_reorder]]
      | ^~~~~~~~~~~~~~~~~~~

unless I wrap your source inside a function.  Was your testcase complete?
Comment 4 Julian Waters 2023-07-22 03:30:56 UTC
My mistake, I forgot to mention that it was indeed inside a function body. I can't seem to figure out what in parser.cc is causing this bug, anything on your end?
Comment 5 Julian Waters 2023-11-23 02:33:05 UTC
Note: Trying this with a top level asm gives me:

$ g++ -O3 -flto=auto -std=c++14 -pedantic -Wpedantic -fno-omit-frame-pointer exceptions.cpp
exceptions.cpp:8:1: error: expected unqualified-id before 'asm'
    8 | asm ("nop");
      | ^~~

So while it seems the errors are different, it fundamentally is the same issue
Comment 6 Julian Waters 2023-11-29 10:45:14 UTC
Created attachment 56717 [details]
Attribute for asm declarations
Comment 7 Julian Waters 2023-11-29 10:47:37 UTC
I have a new attribute proposed for asm declarations that fixes this issue, but I need help from reviews to proceed with fixing up the patch properly
Comment 8 Julian Waters 2023-11-30 14:59:18 UTC
Created attachment 56732 [details]
Attribute for asm declarations
Comment 9 Xi Ruoyao 2023-11-30 16:22:33 UTC
(In reply to Julian Waters from comment #8)
> Created attachment 56732 [details]
> Attribute for asm declarations

Please just fix this issue (i.e. make [[gnu::no_reorder]] work) instead of adding fancy new features.  Now we are in stage 3 of GCC 14 so any fancy new feature should be deferred into stage 1 of GCC 15.
Comment 10 Xi Ruoyao 2023-11-30 16:24:15 UTC
(In reply to Xi Ruoyao from comment #9)
> (In reply to Julian Waters from comment #8)
> > Created attachment 56732 [details]
> > Attribute for asm declarations
> 
> Please just fix this issue (i.e. make [[gnu::no_reorder]] work) instead of
> adding fancy new features.  Now we are in stage 3 of GCC 14 so any fancy new
> feature should be deferred into stage 1 of GCC 15.

Ok, I see you are only using no_reorder as an example.  Then try to make the existing attributes work instead of adding a new one.
Comment 11 Marek Polacek 2023-11-30 16:27:41 UTC
(In reply to Xi Ruoyao from comment #10)
> (In reply to Xi Ruoyao from comment #9)
> > (In reply to Julian Waters from comment #8)
> > > Created attachment 56732 [details]
> > > Attribute for asm declarations
> > 
> > Please just fix this issue (i.e. make [[gnu::no_reorder]] work) instead of
> > adding fancy new features.  Now we are in stage 3 of GCC 14 so any fancy new
> > feature should be deferred into stage 1 of GCC 15.
> 
> Ok, I see you are only using no_reorder as an example.  Then try to make the
> existing attributes work instead of adding a new one.

I agree.  I can hardly see the need for a new and a very complicated attribute.
Comment 12 Julian Waters 2023-12-01 04:52:45 UTC
Will do, will save the new attribute for gcc 15 and just fix the attribute specifier sequence here
Comment 13 Xi Ruoyao 2023-12-01 05:34:45 UTC
Re-titling as there is no such a concept "asm declarations".
Comment 14 Xi Ruoyao 2023-12-01 05:43:44 UTC
Wait a minute.  [[gnu::no_reorder]] is a function or variable attribute, so it's just not intended to be used for an asm statement.  Actually it won't work for any statement:

int x;
int main()
{
	[[gnu::no_reorder]]
	x += 1;
}

gives:

t.c: In function 'main':
t.c:5:9: warning: 'no_reorder' attribute ignored [-Wattributes]
    5 |         x += 1;
      |         ^

We have "statement attributes" (fallthrough and assume) but the doc is clear that they can only be set on null statements, while asm statements are not null statements.

Now to me this is just an invalid PR.  Or am I missing something?
Comment 15 Xi Ruoyao 2023-12-01 05:51:24 UTC
Alright, this is a C++ issue and I mistakenly assumed it was C.
Comment 16 Xi Ruoyao 2023-12-01 05:53:02 UTC
(In reply to Xi Ruoyao from comment #15)
> Alright, this is a C++ issue and I mistakenly assumed it was C.

Note that in C++ we have some inconsistency with the standard.  In the standard asm(...) are "declarations" but in GNU C++ asm(...) are always "statements".
Comment 17 Julian Waters 2023-12-01 06:22:45 UTC
Looking at the source of the C++ parser it doesn't seem like asm (""); is considered a statement but rather is correctly parsed as a declaration, see cp_parser_block_declaration and cp_parser_declaration_statement. The no_reorder attribute is just a demonstration, I am aware it is not meant for asm declarations, rather the error is in how the error message is formed, eg error: expected primary-expression before 'asm' instead of error: expected primary-expression before 'asm'
Comment 18 Julian Waters 2023-12-01 06:23:54 UTC
Oops, I meant warning: 'no_reorder' attribute ignored [-Wattributes] in my above comment
Comment 19 Jakub Jelinek 2023-12-01 06:56:18 UTC
Attributes have been allowed on asm in C++ only since 2017's
https://cplusplus.github.io/CWG/issues/2262.html
which we apparently don't implement.  See
https://gcc.gnu.org/projects/cxx-dr-status.html
It is not clear whether that change has been effectively applied as a DR and thus whether it should apply to earlier standards as well (guess starting with C++11) or not.
Comment 20 Julian Waters 2023-12-02 14:11:35 UTC
cppreference at least seems to indicate it retroactively applies to C++11
https://en.cppreference.com/w/cpp/language/asm
Comment 21 GCC Commits 2023-12-05 16:39:47 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e5153e7d63b4cd9a3df490809c4f3fe1e94d3d37

commit r14-6187-ge5153e7d63b4cd9a3df490809c4f3fe1e94d3d37
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Dec 5 17:38:46 2023 +0100

    c++: Implement C++ DR 2262 - Attributes for asm-definition [PR110734]
    
    Seems in 2017 attribute-specifier-seq[opt] was added to asm-declaration
    and the change was voted in as a DR.
    
    The following patch implements it by parsing the attributes and warning
    about them.
    
    I found one attribute parsing bug I'll send a fix for momentarily.
    
    And there is another thing I wonder about: with -Wno-attributes= we are
    supposed to ignore the attributes altogether, but we are actually still
    warning about them when we emit these generic warnings about ignoring
    all attributes which appertain to this and that (perhaps with some
    exceptions we first remove from the attribute chain), like:
    void foo () { [[foo::bar]]; }
    with -Wattributes -Wno-attributes=foo::bar
    Shouldn't we call some helper function in cases like this and warn
    not when std_attrs (or how the attribute chain var is called) is non-NULL,
    but if it is non-NULL and contains at least one non-attribute_ignored_p
    attribute?  cp_parser_declaration at least tries:
          if (std_attrs != NULL_TREE && !attribute_ignored_p (std_attrs))
            warning_at (make_location (attrs_loc, attrs_loc, parser->lexer),
                        OPT_Wattributes, "attribute ignored");
    but attribute_ignored_p here checks the first attribute rather than the
    whole chain.  So it will incorrectly not warn if there is an ignored
    attribute followed by non-ignored.
    
    2023-12-05  Jakub Jelinek  <jakub@redhat.com>
    
            PR c++/110734
            * parser.cc (cp_parser_block_declaration): Implement C++ DR 2262
            - Attributes for asm-definition.  Call cp_parser_asm_definition
            even if RID_ASM token is only seen after sequence of standard
            attributes.
            (cp_parser_asm_definition): Parse standard attributes before
            RID_ASM token and warn for them with -Wattributes.
    
            * g++.dg/DRs/dr2262.C: New test.
            * g++.dg/cpp0x/gen-attrs-76.C (foo, bar): Don't expect errors
            on attributes on asm definitions.
            * g++.dg/gomp/attrs-11.C: Remove 2 expected errors.
Comment 22 Jakub Jelinek 2023-12-05 17:03:39 UTC
DR now implemented for GCC 12 (but gnu::no_reorder support for namespace scope asms would be more work; I think we don't reorder toplevel asms anyway).
Comment 23 Jakub Jelinek 2023-12-05 17:04:33 UTC
.