[Bug c/60915] New: confusing diagnostic from attribute on function definition

[Bug c/60915] New: confusing diagnostic from attribute on function definition

[tromey at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

            Bug ID: 60915
           Summary: confusing diagnostic from attribute on function
                    definition
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tromey at gcc dot gnu.org

Consider this program:

int something(void) __attribute__((__visibility__("default")))
{
  return 23;
}


Compiling gives:

barimba. gcc --syntax-only t.c
t.c:2:1: error: expected ‘,’ or ‘;’ before ‘{’ token
 {
 ^


A few notes here:

First, while this is not the correct syntax, it seems natural
enough that I've written it several times by mistake now.
Is there any chance it could be blessed?

Second, I think the diagnostic could be improved.  For example
it could mention that an attribute is not valid in this position
on a definition, and suggest an alternative.

Third, the manual could use an example of how to do this.
>From gcc-bugs-return-449412-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Mon Apr 21 14:52:45 2014
Return-Path: <gcc-bugs-return-449412-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 29175 invoked by alias); 21 Apr 2014 14:52:45 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 29133 invoked by uid 48); 21 Apr 2014 14:52:42 -0000
From: "ktietz at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug bootstrap/60830] [4.9 Regression] ICE on bootstrapping on cygwin
Date: Mon, 21 Apr 2014 14:52:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: bootstrap
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords: build
X-Bugzilla-Severity: normal
X-Bugzilla-Who: ktietz at gcc dot gnu.org
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: P1
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 4.9.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-60830-4-sk8UDFYb8b@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-60830-4@http.gcc.gnu.org/bugzilla/>
References: <bug-60830-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-04/txt/msg01432.txt.bz2
Content-length: 1792

http://gcc.gnu.org/bugzilla/show_bug.cgi?id`830

--- Comment #31 from Kai Tietz <ktietz at gcc dot gnu.org> ---
(In reply to Bernd Edlinger from comment #30)
> It looks like a bug in the assembler!
>
> with objdump -d -r crtbegin.o we have wrong code:
>
>   f3:   83 ec 04                sub    $0x4,%esp
>   f6:   85 c0                   test   %eax,%eax
>   f8:   ba f0 ff ff ff          mov    $0xfffffff0,%edx
>                         f9: dir32       ___deregister_frame_info
>   fd:   74 16                   je     115 <___gcc_deregister_frame+0x35>
>
>
> but the cygming-crtbegin.s looks correct:
>
> LVL17:
>         .loc 1 158 0
>         testl   %eax, %eax
>         .loc 1 162 0
>         movl    $___deregister_frame_info, %edx
>         .loc 1 158 0
>         je      L27
>
>
> whenever there is a weak definition the reference is actually minus
> the offset of the previous weak definition. Everything is OK if that was
> at offset zero.
>
> test case:
>
> $ cat test.c
> #include <stdio.h>
>
> extern void test() __attribute__((weak));
>
> int main()
> {
>   printf("hello\n");
>   test();
>   return 0;
> }
>
> __attribute__((weak))
> void test()
> {
>   printf("weak\n");
> }
>
> $ cat test1.c
> #include <stdio.h>
>
> void test()
> {
>   printf("strong\n");
> }
>
>
> $ gcc -o test test.c test1.c
> $ ./test
> hello
> Segmentation fault (core dumped)

Interesting finding.  So second issue cooks down to be an binutils bug about
weak.  I remember the cause of the change to gcc's crtbegin, and it is pretty
important one (at least for x64).  As the implicit zero-relocation doesn't work
on x64 as instructions have +/-2^31 offset in max.
So I would suggest to report this issue on binutils.  gcc's bz isn't the right
place to discuss that issue further.

[Bug c/60915] confusing diagnostic from attribute on function definition

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #1 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Tom Tromey from comment #0)
> First, while this is not the correct syntax, it seems natural
> enough that I've written it several times by mistake now.
> Is there any chance it could be blessed?

It looks like it.  Quoting from
http://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax: "An
attribute specifier list may, in future, be permitted to appear after the
declarator in a function definition (before any old-style parameter
declarations or the function body)."

> Second, I think the diagnostic could be improved.  For example
> it could mention that an attribute is not valid in this position
> on a definition, and suggest an alternative.

Yep.  The following (untested) patch should bring hopefully better error
message; do you agree with the wording?

--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -1688,7 +1688,19 @@ c_parser_declaration_or_fndef (c_parser *parser, bool
fndef_ok,
          if (c_parser_next_token_is_keyword (parser, RID_ASM))
            asm_name = c_parser_simple_asm_expr (parser);
          if (c_parser_next_token_is_keyword (parser, RID_ATTRIBUTE))
-           postfix_attrs = c_parser_attributes (parser);
+           {
+             postfix_attrs = c_parser_attributes (parser);
+             if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
+               {
+                 /* This means there is an attribute specifier after
+                    the declarator in a function definition.  Provide
+                    some more information for the user.  */
+                 error_at (here, "attributes should be specified before the "
+                           "declarator in a function definition");
+                 c_parser_skip_to_end_of_block_or_statement (parser);
+                 return;
+               }
+           }
          if (c_parser_next_token_is (parser, CPP_EQ))
            {
              tree d;

This would of course go away if we decide to support attribute specifier after
the declarator in a function definition.

> Third, the manual could use an example of how to do this.

Yeah, an example never hurts.

[Bug c/60915] confusing diagnostic from attribute on function definition

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Patch posted:
http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01556.html

[Bug c/60915] confusing diagnostic from attribute on function definition

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-04-30
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
   Target Milestone|---                         |4.10.0
     Ever confirmed|0                           |1

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
So mine.

[Bug c/60915] confusing diagnostic from attribute on function definition

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Thu May  1 07:40:26 2014
New Revision: 209975

URL: http://gcc.gnu.org/viewcvs?rev=209975&root=gcc&view=rev
Log:
    PR c/60915
    * c-parser.c (c_parser_declaration_or_fndef): Give better error if
    function-definition has an attribute after the declarator.

    * gcc.dg/pr60915.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr60915.c
Modified:
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-parser.c
    trunk/gcc/testsuite/ChangeLog

[Bug c/60915] confusing diagnostic from attribute on function definition

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed.