[C] Add -Wtentative-definition-compat

[AaronBallman]

This adds a new diagnostic to warn about redeclaration of a tentative definition in C. This is incompatible with C++, so the new diagnostic group is under -Wc++-compat.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

This adds a new diagnostic to warn about redeclaration of a tentative definition in C. This is incompatible with C++, so the new diagnostic group is under -Wc++-compat.

---

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

6 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+9)
• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+3-1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaDecl.cpp (+3)
• (modified) clang/test/Sema/warn-default-const-init.c (+1-1)
• (added) clang/test/Sema/warn-tentative-defn-compat.c (+23)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bc68bb8b70b3d..0c667745f2613 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,15 @@ C Language Changes
   ``-Wenum-conversion`` and ``-Wimplicit-int-enum-cast``. This conversion is an
   int-to-enum conversion because the enumeration on the right-hand side is
   promoted to ``int`` before the assignment.
+- Added ``-Wtentative-definition-compat``, grouped under ``-Wc++-compat``,
+  which diagnoses tentative definitions in C with multiple declarations as
+  being incompatible with C++. e.g.,
+
+  .. code-block:: c
+
+    // File scope
+    int i;
+    int i; // Vaild C, invalid C++, now diagnosed
 
 C2y Feature Support
 ^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index fc1ce197ef134..0ac7b2d39d12e 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -162,8 +162,10 @@ def DefaultConstInit : DiagGroup<"default-const-init", [DefaultConstInitUnsafe]>
 def ImplicitVoidPtrCast : DiagGroup<"implicit-void-ptr-cast">;
 def ImplicitIntToEnumCast : DiagGroup<"implicit-int-enum-cast",
                                       [ImplicitEnumEnumCast]>;
+def TentativeDefnCompat : DiagGroup<"tentative-definition-compat">;
 def CXXCompat: DiagGroup<"c++-compat", [ImplicitVoidPtrCast, DefaultConstInit,
-                                        ImplicitIntToEnumCast, HiddenCppDecl]>;
+                                        ImplicitIntToEnumCast, HiddenCppDecl,
+                                        TentativeDefnCompat]>;
 
 def ExternCCompat : DiagGroup<"extern-c-compat">;
 def KeywordCompat : DiagGroup<"keyword-compat">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ad5bf26be2590..7cba120cbd924 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7443,6 +7443,9 @@ def err_tentative_def_incomplete_type : Error<
 def warn_tentative_incomplete_array : Warning<
   "tentative array definition assumed to have one element">,
   InGroup<DiagGroup<"tentative-definition-array">>;
+def warn_cxx_compat_tentative_definition : Warning<
+  "duplicate declaration of %0 is invalid in C++">,
+  InGroup<TentativeDefnCompat>, DefaultIgnore;
 def err_typecheck_incomplete_array_needs_initializer : Error<
   "definition of variable with array type needs an explicit size "
   "or an initializer">;
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index dfc718eedc1d9..3789eff818e08 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4750,6 +4750,9 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
       if (Def && checkVarDeclRedefinition(Def, New))
         return;
     }
+  } else {
+    Diag(New->getLocation(), diag::warn_cxx_compat_tentative_definition) << New;
+    Diag(Old->getLocation(), diag::note_previous_declaration);
   }
 
   if (haveIncompatibleLanguageLinkages(Old, New)) {
diff --git a/clang/test/Sema/warn-default-const-init.c b/clang/test/Sema/warn-default-const-init.c
index b8da41b333f3d..ed211e366428e 100644
--- a/clang/test/Sema/warn-default-const-init.c
+++ b/clang/test/Sema/warn-default-const-init.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify=c,unsafe -Wdefault-const-init %s
-// RUN: %clang_cc1 -fsyntax-only -verify=c,unsafe -Wc++-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify=c,unsafe -Wc++-compat -Wno-tentative-definition-compat %s
 // RUN: %clang_cc1 -fsyntax-only -verify=unsafe %s
 // RUN: %clang_cc1 -fsyntax-only -verify=c -Wdefault-const-init -Wno-default-const-init-unsafe %s
 // RUN: %clang_cc1 -fsyntax-only -verify=good -Wno-default-const-init-unsafe %s
diff --git a/clang/test/Sema/warn-tentative-defn-compat.c b/clang/test/Sema/warn-tentative-defn-compat.c
new file mode 100644
index 0000000000000..02f3db99992f1
--- /dev/null
+++ b/clang/test/Sema/warn-tentative-defn-compat.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtentative-definition-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wc++-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify=good %s
+// RUN: %clang_cc1 -fsyntax-only -verify=cxx -x c++ %s
+// good-no-diagnostics
+
+int i;      // expected-note {{previous declaration is here}} \
+               cxx-note {{previous definition is here}}
+int i;      // expected-warning {{duplicate declaration of 'i' is invalid in C++}} \
+               cxx-error {{redefinition of 'i'}}
+
+int j = 12; // expected-note {{previous declaration is here}} \
+               cxx-note {{previous definition is here}}
+int j;      // expected-warning {{duplicate declaration of 'j' is invalid in C++}} \
+               cxx-error {{redefinition of 'j'}}
+
+int k;      // expected-note {{previous declaration is here}} \
+               cxx-note {{previous definition is here}}
+int k = 12; // expected-warning {{duplicate declaration of 'k' is invalid in C++}} \
+               cxx-error {{redefinition of 'k'}}
+
+// Cannot have two declarations with initializers, that is a redefinition in
+// both C and C++.

[asmok-g]

@AaronBallman Is it meant that the following fires the warning, because it doesn't look wrong to me:

struct X{};
extern const struct X x;
const struct X x = {};

Here I see:

Tentative definitions
A tentative definition is an external declaration without an initializer, and either without a [storage-class specifier](https://en.cppreference.com/w/c/language/storage_duration) or with the specifier static.

A tentative definition is a declaration that may or may not act as a definition. If an actual external definition is found earlier or later in the same translation unit, then the tentative definition just acts as a declaration.

Sorry if this is actually a naive question.

[asmok-g]

Or even simpler:

extern const int x;
const int x = 0;

[AaronBallman]

Good catch, that looks like a bug to me, it'd be worth reporting it so we have a record.

[asmok-g]

Is there maybe a quick fix or should we add a revert (to revert to green)?

[AaronBallman]

Is there maybe a quick fix or should we add a revert (to revert to green)?

I'm still investigating, so unclear on a quick fix. But to make sure I understand correctly, things are green (no bots are failing, right?), this is just a false positive? Or am I missing something about the severity?

[asmok-g]

By green I'm more referring to the compiler behaving correctly. I think the agreed policy when there's a provided reproducer that proves a certain commit introduces a bug, is that the commit is reverted. It should reland with the bug fixed. The exception is when there's a quick obvious fix.

specifically under When should you revert your own change?

• If a test case that demonstrates a problem is reported in the commit thread, please revert and investigate offline.

[AaronBallman]

I posted a fix: #139738

[asmok-g]

Thanks a lot for the quick fix!