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

[Clang] [Sema] No longer diagnose type definitions in offsetof in C23 mode #84169

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

Sirraide
Copy link
Contributor

@Sirraide Sirraide commented Mar 6, 2024

This is now allowed in C23; continue to diagnose it in earlier language modes as before, but now as a C23 extension rather than a GNU extension.

This fixes #83658.

@Sirraide Sirraide added c23 clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Mar 6, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 6, 2024

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

This is now allowed in C23; continue to diagnose it in earlier language modes as before, but now as a C23 extension rather than a GNU extension.

This fixes #83658.


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

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-2)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+3-1)
  • (modified) clang/test/C/C2x/n2350.c (+5-5)
  • (modified) clang/test/C/drs/dr4xx.c (+10-7)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0ff4a93b15ea8f..8f730c459967be 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -212,6 +212,9 @@ Improvements to Clang's diagnostics
 - Clang now diagnoses lambda function expressions being implicitly cast to boolean values, under ``-Wpointer-bool-conversion``.
   Fixes #GH82512.
 
+- Clang now no longer diagnoses type definitions in ``offsetof`` in C23 mode.
+  Fixes #GH83658.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b007ff7d8ccf0f..bdc9cb9cbca2c7 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1732,8 +1732,8 @@ def err_type_defined_in_condition : Error<
 def err_type_defined_in_enum : Error<
   "%0 cannot be defined in an enumeration">;
 def ext_type_defined_in_offsetof : Extension<
-  "defining a type within '%select{__builtin_offsetof|offsetof}0' is a Clang "
-  "extension">, InGroup<GNUOffsetofExtensions>;
+  "defining a type within '%select{__builtin_offsetof|offsetof}0' is a C23 "
+  "extension">, InGroup<C23>;
 
 def note_pure_virtual_function : Note<
   "unimplemented pure virtual method %0 in %1">;
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 6b81ee183cc440..05347ba297c31c 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -18098,7 +18098,9 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
                                cast_or_null<RecordDecl>(PrevDecl));
   }
 
-  if (OOK != OOK_Outside && TUK == TUK_Definition && !getLangOpts().CPlusPlus)
+  // Only C23 and later allow defining new types in 'offsetof()'.
+  if (OOK != OOK_Outside && TUK == TUK_Definition && !getLangOpts().CPlusPlus &&
+      !getLangOpts().C23)
     Diag(New->getLocation(), diag::ext_type_defined_in_offsetof)
         << (OOK == OOK_Macro) << New->getSourceRange();
 
diff --git a/clang/test/C/C2x/n2350.c b/clang/test/C/C2x/n2350.c
index 2f738488a37427..af0ca6d79be5e1 100644
--- a/clang/test/C/C2x/n2350.c
+++ b/clang/test/C/C2x/n2350.c
@@ -5,7 +5,7 @@
 // RUN: %clang_cc1 -fsyntax-only -pedantic -Wno-comment -std=c99 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -pedantic -Wno-comment -std=c11 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -pedantic -Wno-comment -std=c17 -verify %s
-// RUN: %clang_cc1 -fsyntax-only -pedantic -Wno-comment -std=c2x -verify %s
+// RUN: %clang_cc1 -fsyntax-only -pedantic -Wno-comment -std=c2x -verify=silent %s
 
 // silent-no-diagnostics
 
@@ -13,10 +13,10 @@
 // https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
 int simple(void) {
   return __builtin_offsetof(struct A // cpp-error {{'A' cannot be defined in a type specifier}} \
-                                        expected-warning {{defining a type within '__builtin_offsetof' is a Clang extension}}
+                                        expected-warning {{defining a type within '__builtin_offsetof' is a C23 extension}}
   {
     int a;
-    struct B // expected-warning {{defining a type within '__builtin_offsetof' is a Clang extension}}
+    struct B // expected-warning {{defining a type within '__builtin_offsetof' is a C23 extension}}
     {
       int c;
       int d;
@@ -26,7 +26,7 @@ int simple(void) {
 
 int anonymous_struct(void) {
   return __builtin_offsetof(struct // cpp-error-re {{'(unnamed struct at {{.*}})' cannot be defined in a type specifier}} \
-                                      expected-warning {{defining a type within '__builtin_offsetof' is a Clang extension}}
+                                      expected-warning {{defining a type within '__builtin_offsetof' is a C23 extension}}
   {
     int a;
     int b;
@@ -47,7 +47,7 @@ int struct_in_second_param(void) {
 
 int macro(void) {
   return offsetof(struct A // cpp-error {{'A' cannot be defined in a type specifier}} \
-                              expected-warning 2 {{defining a type within 'offsetof' is a Clang extension}}
+                              expected-warning 2 {{defining a type within 'offsetof' is a C23 extension}}
   {
     int a;
     struct B // verifier seems to think the error is emitted by the macro
diff --git a/clang/test/C/drs/dr4xx.c b/clang/test/C/drs/dr4xx.c
index 30145dcfeef168..83d7b94cd67959 100644
--- a/clang/test/C/drs/dr4xx.c
+++ b/clang/test/C/drs/dr4xx.c
@@ -1,7 +1,7 @@
-/* RUN: %clang_cc1 -std=c89 -verify=expected,c89only -pedantic -Wno-c11-extensions %s
-   RUN: %clang_cc1 -std=c99 -verify=expected -pedantic -Wno-c11-extensions %s
-   RUN: %clang_cc1 -std=c11 -verify=expected -pedantic %s
-   RUN: %clang_cc1 -std=c17 -verify=expected -pedantic %s
+/* RUN: %clang_cc1 -std=c89 -verify=expected,c89only,pre-c23 -pedantic -Wno-c11-extensions %s
+   RUN: %clang_cc1 -std=c99 -verify=expected,pre-c23 -pedantic -Wno-c11-extensions %s
+   RUN: %clang_cc1 -std=c11 -verify=expected,pre-c23 -pedantic %s
+   RUN: %clang_cc1 -std=c17 -verify=expected,pre-c23 -pedantic %s
    RUN: %clang_cc1 -std=c2x -verify=expected -pedantic %s
  */
 
@@ -343,10 +343,13 @@ void dr496(void) {
                                              */
 
   /* The DR asked a question about whether defining a new type within offsetof
-   * is allowed. C2x N2350 made this explicitly undefined behavior, but GCC and
-   * Clang both support it as an extension.
+   * is allowed. C23 N2350 had made this explicitly undefined behavior, but this
+   * was later overturned when C23 DE-137 was accepted, making it well-formed.
+   *
+   * Additionally, GCC and Clang both support it as an extension in pre-C23
+   * mode.
    */
-   (void)__builtin_offsetof(struct S { int a; }, a); /* expected-warning{{defining a type within '__builtin_offsetof' is a Clang extension}} */
+   (void)__builtin_offsetof(struct S { int a; }, a); /* pre-c23-warning{{defining a type within '__builtin_offsetof' is a C23 extension}} */
 }
 
 /* WG14 DR499: yes

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we verify that we diagnose the case where the definition includes a comma?

@Sirraide
Copy link
Contributor Author

Sirraide commented Mar 8, 2024

Should we verify that we diagnose the case where the definition includes a comma?

It’s UB if the type contains a comma (unless I’m somehow looking at the wrong C standard again...), so I don’t think we need to diagnose it.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Sirraide Sirraide merged commit af34ac4 into llvm:main Mar 29, 2024
4 of 5 checks passed
@Sirraide Sirraide deleted the 83658 branch March 29, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c23 clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type definitions should be allowed inside of the offsetof type
4 participants