[C] Disallow declarations where a statement is required #92908

AaronBallman · 2024-05-21T12:45:27Z

This fixes a regression introduced in 8bd06d5 where Clang began to accept a declaration where a statement is required. e.g.,

if (1)
  int x; // Previously accepted, now properly rejected

Fixes #92775

8bd06d5 removed the notion of parsed statement contexts that track whether a declaration is valid for C or not. This resurrects the concept, because C still does not allow a declaration as a substatement.

…n-parsing

llvmbot · 2024-05-21T12:46:02Z

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

This fixes a regression introduced in 8bd06d5 where Clang began to accept a declaration where a statement is required. e.g.,

if (1)
  int x; // Previously accepted, now properly rejected

Fixes #92775

Full diff: https://github.com/llvm/llvm-project/pull/92908.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/include/clang/Parse/Parser.h (+6-3)
(modified) clang/lib/Parse/ParseStmt.cpp (+4-1)
(modified) clang/test/C/C99/block-scopes.c (+2-1)
(added) clang/test/Parser/decls.c (+39)
(modified) clang/test/SemaOpenACC/parallel-loc-and-stmt.c (+4-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 81e9d0423f96a..a47252c536c0f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -589,6 +589,9 @@ Bug Fixes in This Version
 - ``__is_array`` and ``__is_bounded_array`` no longer return ``true`` for
   zero-sized arrays. Fixes (#GH54705).
 
+- Correctly reject declarations where a statement is required in C.
+  Fixes #GH92775
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 5f04664141d29..5e455b16fb8c0 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -467,15 +467,18 @@ class Parser : public CodeCompletionHandler {
 
   /// Flags describing a context in which we're parsing a statement.
   enum class ParsedStmtContext {
+    /// This context permits declarations in language modes where declarations
+    /// are not statements.
+    AllowDeclarationsInC = 0x1,
     /// This context permits standalone OpenMP directives.
-    AllowStandaloneOpenMPDirectives = 0x1,
+    AllowStandaloneOpenMPDirectives = 0x2,
     /// This context is at the top level of a GNU statement expression.
-    InStmtExpr = 0x2,
+    InStmtExpr = 0x4,
 
     /// The context of a regular substatement.
     SubStmt = 0,
     /// The context of a compound-statement.
-    Compound = AllowStandaloneOpenMPDirectives,
+    Compound = AllowDeclarationsInC | AllowStandaloneOpenMPDirectives,
 
     LLVM_MARK_AS_BITMASK_ENUM(InStmtExpr)
   };
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index b0af04451166c..57b9fb10a1e08 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -239,7 +239,10 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
     auto IsStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); };
     bool AllAttrsAreStmtAttrs = llvm::all_of(CXX11Attrs, IsStmtAttr) &&
                                 llvm::all_of(GNUAttrs, IsStmtAttr);
-    if (((GNUAttributeLoc.isValid() && !(HaveAttrs && AllAttrsAreStmtAttrs)) ||
+    if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
+         (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
+             ParsedStmtContext()) &&
+        ((GNUAttributeLoc.isValid() && !(HaveAttrs && AllAttrsAreStmtAttrs)) ||
          isDeclarationStatement())) {
       SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
       DeclGroupPtrTy Decl;
diff --git a/clang/test/C/C99/block-scopes.c b/clang/test/C/C99/block-scopes.c
index 589047df3e52b..116e5d922593e 100644
--- a/clang/test/C/C99/block-scopes.c
+++ b/clang/test/C/C99/block-scopes.c
@@ -18,8 +18,9 @@
 
 enum {a, b};
 void different(void) {
-  if (sizeof(enum {b, a}) != sizeof(int))
+  if (sizeof(enum {b, a}) != sizeof(int)) {
     _Static_assert(a == 1, "");
+  }
   /* In C89, the 'b' found here would have been from the enum declaration in
    * the controlling expression of the selection statement, not from the global
    * declaration. In C99 and later, that enumeration is scoped to the 'if'
diff --git a/clang/test/Parser/decls.c b/clang/test/Parser/decls.c
new file mode 100644
index 0000000000000..39ef05bf4bd99
--- /dev/null
+++ b/clang/test/Parser/decls.c
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic
+
+// Test that we can parse declarations at global scope.
+int v;
+
+void func(void) {
+  // Test that we can parse declarations within a compound statement.
+  int a;
+  {
+    int b;
+  }
+
+  int z = ({ // expected-warning {{use of GNU statement expression extension}}
+	// Test that we can parse declarations within a GNU statement expression.
+	int w = 12;
+	w;
+  });
+
+  // Test that we diagnose declarations where a statement is required.
+  // See GH92775.
+  if (1)
+    int x; // expected-error {{expected expression}}
+  for (;;)
+    int c; // expected-error {{expected expression}}
+
+  label:
+    int y; // expected-warning {{label followed by a declaration is a C23 extension}}
+
+  // Test that lookup works as expected.
+  (void)a;
+  (void)v;
+  (void)z;
+  (void)b; // expected-error {{use of undeclared identifier 'b'}}
+  (void)w; // expected-error {{use of undeclared identifier 'w'}}
+  (void)x; // expected-error {{use of undeclared identifier 'x'}}
+  (void)c; // expected-error {{use of undeclared identifier 'c'}}
+  (void)y;
+}
+
diff --git a/clang/test/SemaOpenACC/parallel-loc-and-stmt.c b/clang/test/SemaOpenACC/parallel-loc-and-stmt.c
index ba29f6da8ba25..bbcdd823483a5 100644
--- a/clang/test/SemaOpenACC/parallel-loc-and-stmt.c
+++ b/clang/test/SemaOpenACC/parallel-loc-and-stmt.c
@@ -33,9 +33,11 @@ int foo3;
 
 void func() {
   // FIXME: Should we disallow this on declarations, or consider this to be on
-  // the initialization?
+  // the initialization? This is currently rejected in C because
+  // Parser::ParseOpenACCDirectiveStmt() calls ParseStatement() and passes the
+  // statement context as "SubStmt" which does not allow for a declaration in C.
 #pragma acc parallel
-  int foo;
+  int foo; // expected-error {{expected expression}}
 
 #pragma acc parallel
   {

erichkeane

The original patch changed some of the ParsedStmtContext's in ParseOMP that might need to be changed back? Looking back, I think the intent there was in part to not specify OMP standalone directives, so that part should be reverted as well perhaps?

AaronBallman · 2024-05-21T13:44:55Z

I'm less certain, but CC @jdoerfert @mikerice1969 for more opinions.

The original changes actually fixed a bug in OpenMP related to standalone directives.

Note, this seems to have fixed an issue with some OpenMP stand-alone
directives not being properly diagnosed as per:
https://www.openmp.org/spec-html/5.1/openmpsu19.html#x34-330002.1.3
(The same requirement exists in OpenMP 5.2 as well.)

shafik · 2024-05-21T19:26:26Z

clang/include/clang/Parse/Parser.h

@@ -467,15 +467,18 @@ class Parser : public CodeCompletionHandler {

  /// Flags describing a context in which we're parsing a statement.
  enum class ParsedStmtContext {
+    /// This context permits declarations in language modes where declarations
+    /// are not statements.
+    AllowDeclarationsInC = 0x1,


This feels a little mysterious, I only see that we check it but it is not obvious how it gets set.

@shafik L481 below

AaronBallman · 2024-05-22T11:37:20Z

It gets set a few lines down for Compound:

Compound = AllowDeclarationsInC | AllowStandaloneOpenMPDirectives

cor3ntin

LGTM modulo comment

cor3ntin · 2024-05-23T17:11:37Z

clang/include/clang/Parse/Parser.h

@@ -467,15 +467,18 @@ class Parser : public CodeCompletionHandler {

  /// Flags describing a context in which we're parsing a statement.
  enum class ParsedStmtContext {
+    /// This context permits declarations in language modes where declarations
+    /// are not statements.
+    AllowDeclarationsInC = 0x1,


@shafik L481 below

cor3ntin · 2024-05-23T17:12:24Z

clang/lib/Parse/ParseStmt.cpp

+    if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
+         (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
+             ParsedStmtContext()) &&
+        ((GNUAttributeLoc.isValid() && !(HaveAttrs && AllAttrsAreStmtAttrs)) ||


Can you add a standard quote?

I've added one, let me know if you're happy with it.

Thanks (somehow i did not get notified)

…n-parsing

This fixes a regression introduced in 8bd06d5 where Clang began to accept a declaration where a statement is required. e.g., ``` if (1) int x; // Previously accepted, now properly rejected ``` Fixes llvm#92775

AaronBallman added 7 commits May 20, 2024 13:40

[C] Fix declaration parsing

219dae0

8bd06d5 removed the notion of parsed statement contexts that track whether a declaration is valid for C or not. This resurrects the concept, because C still does not allow a declaration as a substatement.

Merge remote-tracking branch 'origin/main' into aballman-c-declaratio…

12c89e0

…n-parsing

Merge remote-tracking branch 'origin/main' into aballman-c-declaratio…

c4b0d42

…n-parsing

Merge remote-tracking branch 'origin/main' into aballman-c-declaratio…

9a21719

…n-parsing

Update OpenACC test for slight behavioral change

c67502d

Add test coverage for changes

6e2e9c0

Add release note

f59942f

AaronBallman added c clang:frontend Language frontend issues, e.g. anything involving "Sema" accepts-invalid labels May 21, 2024

AaronBallman requested review from jrtc27, jyknight and erichkeane May 21, 2024 12:45

llvmbot added the clang Clang issues not falling into any other category label May 21, 2024

erichkeane reviewed May 21, 2024

View reviewed changes

shafik reviewed May 21, 2024

View reviewed changes

cor3ntin approved these changes May 23, 2024

View reviewed changes

AaronBallman added 2 commits May 28, 2024 09:38

Merge remote-tracking branch 'origin/main' into aballman-c-declaratio…

0639525

…n-parsing

Add a comment with some standards citations; NFC

3f00614

AaronBallman merged commit 5901d40 into llvm:main May 28, 2024
8 checks passed

AaronBallman deleted the aballman-c-declaration-parsing branch May 28, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[C] Disallow declarations where a statement is required #92908

[C] Disallow declarations where a statement is required #92908

Uh oh!

AaronBallman commented May 21, 2024

Uh oh!

llvmbot commented May 21, 2024

Uh oh!

erichkeane left a comment

Uh oh!

AaronBallman commented May 21, 2024

Uh oh!

shafik May 21, 2024

Uh oh!

cor3ntin May 23, 2024

Uh oh!

AaronBallman commented May 22, 2024

Uh oh!

cor3ntin left a comment

Uh oh!

cor3ntin May 23, 2024

Uh oh!

cor3ntin May 23, 2024

Uh oh!

AaronBallman May 28, 2024

Uh oh!

cor3ntin May 28, 2024

Uh oh!

Uh oh!

Uh oh!

[C] Disallow declarations where a statement is required #92908

[C] Disallow declarations where a statement is required #92908

Uh oh!

Conversation

AaronBallman commented May 21, 2024

Uh oh!

llvmbot commented May 21, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented May 21, 2024

Uh oh!

shafik May 21, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 23, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented May 22, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 23, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 23, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman May 28, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!