Bug 88766

Summary: [9 Regression] Rejects valid? C code since r259641
Product: gcc Reporter: Martin Liška <marxin>
Component: cAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED INVALID    
Severity: normal CC: jakub, jsm28, msebor
Priority: P3 Keywords: rejects-valid
Version: 9.0   
Target Milestone: 9.0   
See Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88773
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed:

Description Martin Liška 2019-01-09 10:26:08 UTC
Following code (reduced from gpg2) now fails to compile:

$ cat dns-stuff.i
struct dns_options {
  struct {
    void *a;
    int b;
  };
  int *socks_host;
  char *socks_user;
  char *socks_password;
};
static char tor_socks_user[1], tor_socks_password[1];
struct {
  int socks_host;
} libdns;
int d;
int *c();
int ax() {
  int *az;
  int ba;
  az = c((&__extension__({
           (struct dns_options){{0, 0},
                                0,
                                0,
                                .socks_host = &libdns.socks_host,
                                .socks_user = tor_socks_user,
                                .socks_password = tor_socks_password};
         })),
         &ba);
  d = *az;
  return 0;
}

$ gcc dns-stuff.i
dns-stuff.i: In function ‘ax’:
dns-stuff.i:19:11: error: lvalue required as unary ‘&’ operand
   19 |   az = c((&__extension__({
      |           ^
Comment 1 Jakub Jelinek 2019-01-09 11:39:05 UTC
Reduced testcase:

struct S { int s; };

void
foo (void)
{
  void *p = &(struct S) { 0 };
  void *q = &({ (struct S) { 0 }; });
}

The p initializer is accepted, q is rejected.
By my reading this is invalid, C99 6.5.2.5/6 says:
"If the compound literal occurs outside the body of a function, the object
has static storage duration; otherwise, it has automatic storage duration associated with the enclosing block."
and the statement expression is still a compound statement and thus the compound literal is associated with the statement expression's block.  So it is the same thing as:
void
bar (void)
{
  void *r = &({ int a = 0; a; });
}
which fails with the same diagnostics.

Joseph, do you agree?
Comment 2 jsm-csl@polyomino.org.uk 2019-01-09 16:58:57 UTC
Yes, I think that (a) a statement expression is not an lvalue and (b) if 
it were (or if the code were changed to move the unary '&' inside the 
statement expression), the code would be taking the address of an object 
whose lifetime had ended by the time that address is used.
Comment 3 Jakub Jelinek 2019-01-09 17:17:52 UTC
gpg2 needs to be fixed then.

Looking at the source, they wrap the complit in:
#define DNS_PRAGMA_PUSH _Pragma("GCC diagnostic push")
#define DNS_PRAGMA_QUIET _Pragma("GCC diagnostic ignored \"-Woverride-init\"")
#define DNS_PRAGMA_POP _Pragma("GCC diagnostic pop")
 
/* GCC parses the _Pragma operator less elegantly than clang. */
#define dns_quietinit(...) \
        __extension__ ({ DNS_PRAGMA_PUSH DNS_PRAGMA_QUIET __VA_ARGS__; DNS_PRAGMA_POP })

while for clang they use
#define dns_quietinit(...) \
        DNS_PRAGMA_PUSH DNS_PRAGMA_QUIET __VA_ARGS__ DNS_PRAGMA_POP

Trying:
#define DNS_PRAGMA_PUSH _Pragma("GCC diagnostic push")
#define DNS_PRAGMA_QUIET _Pragma("GCC diagnostic ignored \"-Woverride-init\"")
#define DNS_PRAGMA_POP _Pragma("GCC diagnostic pop")

#define dns_quietinit(...) \
        DNS_PRAGMA_PUSH DNS_PRAGMA_QUIET __VA_ARGS__ DNS_PRAGMA_POP

struct S { int a, b; };

void
foo (void)
{
  void *p = &(struct S) { .a = 0, .b = 1, .a = 0 };
  void *q = &dns_quietinit ((struct S) { .a = 0, .b = 1, .a = 0 });
}
indeed fails to parse:
/tmp/h.c: In function ‘foo’:
/tmp/h.c:13:48: warning: initialized field overwritten [-Woverride-init]
   void *p = &(struct S) { .a = 0, .b = 1, .a = 0 };
                                                ^
/tmp/h.c:13:48: note: (near initialization for ‘(anonymous).a’)
/tmp/h.c:14:1: error: expected expression before ‘#pragma’
   void *q = &dns_quietinit ((struct S) { .a = 0, .b = 1, .a = 0 });
 ^ ~
(the line 13 warnings are expected and line 14 is their attempt to disable the warning.  We don't really support pragmas in the middle of expressions, but a compound literal can't be used here.
Comment 4 Martin Sebor 2019-01-09 17:25:53 UTC
I don't disagree with the conclusion about the validity of this test case but there are examples where GCC does treat a statement expression as an lvalue, such as in the following:

  void f (void)
  { 
    int i = 0;
    ++__extension__({ (int)i; });   // accepted
    if (i != 1)                     // folded to false
      __builtin_abort ();
  }

Accepting this seems like a bug irrespective of whether statement expressions themselves are ever lvalues.

GCC of course also accepts ++({ i; }).  Joseph's comment #2 suggests this should be invalid as well but I might be reading too much into it.  In any case, it would be helpful to make this clear in the manual.
Comment 5 jsm-csl@polyomino.org.uk 2019-01-09 17:32:44 UTC
On Wed, 9 Jan 2019, msebor at gcc dot gnu.org wrote:

> I don't disagree with the conclusion about the validity of this test case but
> there are examples where GCC does treat a statement expression as an lvalue,
> such as in the following:
> 
>   void f (void)
>   { 
>     int i = 0;
>     ++__extension__({ (int)i; });   // accepted
>     if (i != 1)                     // folded to false
>       __builtin_abort ();
>   }
> 
> Accepting this seems like a bug irrespective of whether statement expressions
> themselves are ever lvalues.

Indeed.  Casts as lvalues were removed a very long time ago.

> GCC of course also accepts ++({ i; }).  Joseph's comment #2 suggests this
> should be invalid as well but I might be reading too much into it.  In any
> case, it would be helpful to make this clear in the manual.

I think ++({ i; }) should be invalid.  (If i is an _Atomic int, it's 
already rejected because of the special lvalue-to-rvalue processing 
applied for atomics.)
Comment 6 Martin Sebor 2019-01-09 18:03:57 UTC
Okay, I've raised pr88773 for these problems.
Comment 7 Jakub Jelinek 2019-01-09 18:10:56 UTC
As for the GCC diagnostic pragma, we need it to be a deferred pragma, have no idea how else could we handle that when it is not visible in the token stream.  And, if it is visible in the token stream, accepting it everywhere in the exceptions would be quite hard too, e.g. the C FE normally has only 2 tokens lookahead, or how would it play in C++ FE with the tentative parsing?
What we could do is accept #pragma GCC diagnostic (and only that?) inside of the braced initializers (so c_parser_braced_init for C and cp_parser_braced_list? for C++), before each init elt and after those, perhaps rejecting it if it appears before a comma.  Of course, for gnupg2 they would need to adjust their macros slightly, because the would need to go inside of the compound literal braced init rather than around it.

Thoughts on that?
Comment 8 Jakub Jelinek 2019-01-09 18:20:22 UTC
A C proof of concept:
--- gcc/c/c-parser.c.jj	2019-01-07 09:47:33.187515618 +0100
+++ gcc/c/c-parser.c	2019-01-09 19:15:05.216756760 +0100
@@ -4624,6 +4624,11 @@ c_parser_braced_init (c_parser *parser,
     }
   else
     really_start_incremental_init (type);
+
+  /* Accept #pragmas at braced init scope.  */
+  while (c_parser_next_token_is (parser, CPP_PRAGMA))
+    c_parser_pragma (parser, pragma_struct, NULL);
+
   if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
     {
       pedwarn (brace_loc, OPT_Wpedantic, "ISO C forbids empty initializer braces");
@@ -4634,6 +4639,10 @@ c_parser_braced_init (c_parser *parser,
 	 comma.  */
       while (true)
 	{
+	  /* Accept #pragmas at braced init scope.  */
+	  while (c_parser_next_token_is (parser, CPP_PRAGMA))
+	    c_parser_pragma (parser, pragma_struct, NULL);
+
 	  c_parser_initelt (parser, &braced_init_obstack);
 	  if (parser->error)
 	    break;
@@ -4644,10 +4653,20 @@ c_parser_braced_init (c_parser *parser,
 	    }
 	  else
 	    break;
+
+	  /* Accept #pragmas at braced init scope.  */
+	  while (c_parser_next_token_is (parser, CPP_PRAGMA))
+	    c_parser_pragma (parser, pragma_struct, NULL);
+
 	  if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
 	    break;
 	}
     }
+
+  /* Accept #pragmas at braced init scope.  */
+  while (c_parser_next_token_is (parser, CPP_PRAGMA))
+    c_parser_pragma (parser, pragma_struct, NULL);
+
   c_token *next_tok = c_parser_peek_token (parser);
   if (next_tok->type != CPP_CLOSE_BRACE)
     {

Of course, this one parses all pragmas, rather than just GCC diagnostic, and for most pragmas handling them in this context is undesirable (either meaningful, or e.g. OpenMP/OpenACC forbids them there).