[C] Warn on uninitialized const objects #137166
Conversation
The unsafe variant is on by default, the other one is not.
Without this change, we parse as though the initial declaration is an uninitialized variable. With this change, we behave the same as we do for range-based for loops in C++, which is not handled as an uninitialized variable. Note, ParseDeclGroup (which is what is eventually called as a result of this change) was already expecting to be able to handle for...in loops, so this appears to be fixing an oversight.
AaronBallman
added
c
clang:frontend
AaronBallman
requested review from
jyknight,
erichkeane,
efriedma-quic and
Sirraide
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesUnlike C++, C allows the definition of an uninitialized This adds a new diagnostic group, It also adds another new diagnostic group, Finally, it adds Fixes #19297 Patch is 28.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137166.diff 28 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f24fb23d44d..d974c0b83560e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -140,6 +140,13 @@ C Language Changes
- Clang now allows an ``inline`` specifier on a typedef declaration of a
function type in Microsoft compatibility mode. #GH124869
- Clang now allows ``restrict`` qualifier for array types with pointer elements (#GH92847).
+- Clang now diagnoses ``const``-qualified object definitions without an
+ initializer. If the object is zero-initialized, it will be diagnosed under
+ the new warning ``-Wdefault-const-init`` (which is grouped under
+ ``-Wc++-compat`` because this construct is not compatible with C++). If the
+ object is left uninitialized, it will be diagnosed unsed the new warning
+ ``-Wdefault-const-init-unsafe`` (which is grouped under
+ ``-Wdefault-const-init``). #GH19297
- Added ``-Wimplicit-void-ptr-cast``, grouped under ``-Wc++-compat``, which
diagnoses implicit conversion from ``void *`` to another pointer type as
being incompatible with C++. (#GH17792)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 6441b8049ed8d..31e2cfa7ab485 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -154,9 +154,11 @@ def BuiltinRequiresHeader : DiagGroup<"builtin-requires-header">;
def C99Compat : DiagGroup<"c99-compat">;
def C23Compat : DiagGroup<"c23-compat">;
def : DiagGroup<"c2x-compat", [C23Compat]>;
-
+def DefaultConstInitUnsafe : DiagGroup<"default-const-init-unsafe">;
+def DefaultConstInit : DiagGroup<"default-const-init", [DefaultConstInitUnsafe]>;
def ImplicitVoidPtrCast : DiagGroup<"implicit-void-ptr-cast">;
-def CXXCompat: DiagGroup<"c++-compat", [ImplicitVoidPtrCast]>;
+def CXXCompat: DiagGroup<"c++-compat", [ImplicitVoidPtrCast, DefaultConstInit]>;
+
def ExternCCompat : DiagGroup<"extern-c-compat">;
def KeywordCompat : DiagGroup<"keyword-compat">;
def GNUCaseRange : DiagGroup<"gnu-case-range">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8ff170520aafe..7090cbe7acbe6 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8197,6 +8197,16 @@ def err_address_space_qualified_new : Error<
def err_address_space_qualified_delete : Error<
"'delete' cannot delete objects of type %0 in address space '%1'">;
+def note_default_init_const_member : Note<
+ "member %0 declared 'const' here">;
+def warn_default_init_const : Warning<
+ "default initialization of an object of type %0%select{| with const member}1 "
+ "is incompatible with C++">,
+ InGroup<DefaultConstInit>, DefaultIgnore;
+def warn_default_init_const_unsafe : Warning<
+ "default initialization of an object of type %0%select{| with const member}1 "
+ "leaves the object uninitialized and is incompatible with C++">,
+ InGroup<DefaultConstInitUnsafe>;
def err_default_init_const : Error<
"default initialization of an object of const type %0"
"%select{| without a user-provided default constructor}1">;
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 4a82d57fe566b..4e801f4ef890f 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -2154,7 +2154,7 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
FirstPart = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
} else {
// In C++0x, "for (T NS:a" might not be a typo for ::
- bool MightBeForRangeStmt = getLangOpts().CPlusPlus;
+ bool MightBeForRangeStmt = getLangOpts().CPlusPlus || getLangOpts().ObjC;
ColonProtectionRAIIObject ColonProtection(*this, MightBeForRangeStmt);
ParsedAttributes DeclSpecAttrs(AttrFactory);
DG = ParseSimpleDeclaration(
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 4039601612c62..87670fe1953db 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1448,6 +1448,19 @@ void Sema::ActOnEndOfTranslationUnit() {
// No initialization is performed for a tentative definition.
CheckCompleteVariableDeclaration(VD);
+ // In C, if the definition is const-qualified and has no initializer, it
+ // is left uninitialized unless it has static or thread storage duration.
+ QualType Type = VD->getType();
+ if (!VD->isInvalidDecl() && !getLangOpts().CPlusPlus &&
+ Type.isConstQualified() && !VD->getAnyInitializer()) {
+ unsigned DiagID = diag::warn_default_init_const_unsafe;
+ if (VD->getStorageDuration() == SD_Static ||
+ VD->getStorageDuration() == SD_Thread)
+ DiagID = diag::warn_default_init_const;
+ Diag(VD->getLocation(), DiagID) << Type << /*not a field*/ 0;
+ }
+
+
// Notify the consumer that we've completed a tentative definition.
if (!VD->isInvalidDecl())
Consumer.CompleteTentativeDefinition(VD);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index d28a2107d58a9..fa1f3bc732338 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -14333,6 +14333,16 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
return;
}
+ // In C, if the definition is const-qualified and has no initializer, it
+ // is left uninitialized unless it has static or thread storage duration.
+ if (!getLangOpts().CPlusPlus && Type.isConstQualified()) {
+ unsigned DiagID = diag::warn_default_init_const_unsafe;
+ if (Var->getStorageDuration() == SD_Static ||
+ Var->getStorageDuration() == SD_Thread)
+ DiagID = diag::warn_default_init_const;
+ Diag(Var->getLocation(), DiagID) << Type << /*not a field*/ 0;
+ }
+
// Check for jumps past the implicit initializer. C++0x
// clarifies that this applies to a "variable with automatic
// storage duration", not a "local variable".
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index f04a154bcfd5f..ddc4070346265 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -6496,6 +6496,17 @@ static bool canPerformArrayCopy(const InitializedEntity &Entity) {
return false;
}
+static const FieldDecl *GetConstField(const RecordDecl *RD) {
+ for (const FieldDecl *FD : RD->fields()) {
+ QualType QT = FD->getType();
+ if (QT.isConstQualified())
+ return FD;
+ if (const auto *RD = QT->getAsRecordDecl())
+ return GetConstField(RD);
+ }
+ return nullptr;
+}
+
void InitializationSequence::InitializeFrom(Sema &S,
const InitializedEntity &Entity,
const InitializationKind &Kind,
@@ -6563,13 +6574,28 @@ void InitializationSequence::InitializeFrom(Sema &S,
if (!S.getLangOpts().CPlusPlus &&
Kind.getKind() == InitializationKind::IK_Default) {
- RecordDecl *Rec = DestType->getAsRecordDecl();
- if (Rec && Rec->hasUninitializedExplicitInitFields()) {
+ if (RecordDecl *Rec = DestType->getAsRecordDecl()) {
VarDecl *Var = dyn_cast_or_null<VarDecl>(Entity.getDecl());
- if (Var && !Initializer) {
- S.Diag(Var->getLocation(), diag::warn_field_requires_explicit_init)
- << /* Var-in-Record */ 1 << Rec;
- emitUninitializedExplicitInitFields(S, Rec);
+ if (Rec->hasUninitializedExplicitInitFields()) {
+ if (Var && !Initializer) {
+ S.Diag(Var->getLocation(), diag::warn_field_requires_explicit_init)
+ << /* Var-in-Record */ 1 << Rec;
+ emitUninitializedExplicitInitFields(S, Rec);
+ }
+ }
+ // If the record has any members which are const (recursively checked),
+ // then we want to diagnose those as being uninitialized if there is no
+ // initializer present.
+ if (!Initializer) {
+ if (const FieldDecl *FD = GetConstField(Rec)) {
+ unsigned DiagID = diag::warn_default_init_const_unsafe;
+ if (Var->getStorageDuration() == SD_Static ||
+ Var->getStorageDuration() == SD_Thread)
+ DiagID = diag::warn_default_init_const;
+
+ S.Diag(Var->getLocation(), DiagID) << Var->getType() << /*member*/ 1;
+ S.Diag(FD->getLocation(), diag::note_default_init_const_member) << FD;
+ }
}
}
}
diff --git a/clang/test/C/C23/n2607.c b/clang/test/C/C23/n2607.c
index 9595aaed54c43..41e8b2ac4b2cd 100644
--- a/clang/test/C/C23/n2607.c
+++ b/clang/test/C/C23/n2607.c
@@ -24,7 +24,7 @@ void test1(void) {
void test2(void) {
typedef int array[1];
array reg_array;
- const array const_array;
+ const array const_array = { 0 };
// An array and its elements are identically qualified. We have to test this
// using pointers to the array and element, because the controlling
@@ -50,7 +50,7 @@ void test2(void) {
void test3(void) {
// Validate that we pick the correct composite type for a conditional
// operator in the presence of qualifiers.
- const int const_array[1];
+ const int const_array[1] = { 0 };
int array[1];
// FIXME: the type here should be `const int (*)[1]`, but for some reason,
diff --git a/clang/test/C/drs/dr1xx.c b/clang/test/C/drs/dr1xx.c
index 47538e44428c3..3e4c39cca62e4 100644
--- a/clang/test/C/drs/dr1xx.c
+++ b/clang/test/C/drs/dr1xx.c
@@ -289,7 +289,7 @@ void dr124(void) {
*/
void dr126(void) {
typedef int *IP;
- const IP object; /* expected-note {{variable 'object' declared const here}} */
+ const IP object = 0; /* expected-note {{variable 'object' declared const here}} */
/* The root of the DR is whether 'object' is a pointer to a const int, or a
* const pointer to int.
@@ -329,7 +329,7 @@ void dr129(void) {
void dr131(void) {
struct S {
const int i; /* expected-note {{data member 'i' declared const here}} */
- } s1, s2;
+ } s1 = { 0 }, s2 = { 0 };
s1 = s2; /* expected-error {{cannot assign to variable 's1' with const-qualified data member 'i'}} */
}
diff --git a/clang/test/Parser/typeof.c b/clang/test/Parser/typeof.c
index 08f3ca72ab942..f962bcf143957 100644
--- a/clang/test/Parser/typeof.c
+++ b/clang/test/Parser/typeof.c
@@ -12,8 +12,8 @@ static void test(void) {
short TInt eee; // expected-error{{expected ';' at end of declaration}}
void ary[7] fff; // expected-error{{array has incomplete element type 'void'}} expected-error{{expected ';' at end of declaration}}
typeof(void ary[7]) anIntError; // expected-error{{expected ')'}} expected-note {{to match this '('}} expected-error {{variable has incomplete type 'typeof(void)' (aka 'void')}}
- typeof(const int) aci;
- const typeof (*pi) aConstInt;
+ typeof(const int) aci = 0;
+ const typeof (*pi) aConstInt = 0;
int xx;
int *i;
}
diff --git a/clang/test/Sema/assign.c b/clang/test/Sema/assign.c
index fe2a9f1434479..13ea1062b9e1e 100644
--- a/clang/test/Sema/assign.c
+++ b/clang/test/Sema/assign.c
@@ -11,8 +11,8 @@ void test2 (const struct {int a;} *x) {
typedef int arr[10];
void test3(void) {
- const arr b; // expected-note {{variable 'b' declared const here}}
- const int b2[10]; // expected-note {{variable 'b2' declared const here}}
+ const arr b = {}; // expected-note {{variable 'b' declared const here}}
+ const int b2[10] = {}; // expected-note {{variable 'b2' declared const here}}
b[4] = 1; // expected-error {{cannot assign to variable 'b' with const-qualified type 'const arr' (aka 'const int[10]')}}
b2[4] = 1; // expected-error {{cannot assign to variable 'b2' with const-qualified type 'const int[10]'}}
}
diff --git a/clang/test/Sema/atomic-ops.c b/clang/test/Sema/atomic-ops.c
index 725a12060d4e0..aae7aced2628a 100644
--- a/clang/test/Sema/atomic-ops.c
+++ b/clang/test/Sema/atomic-ops.c
@@ -1,20 +1,20 @@
// RUN: %clang_cc1 %s -verify=expected,fp80,noi128 -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=i686-linux-gnu -std=c11
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=i686-linux-gnu -std=c11
// RUN: %clang_cc1 %s -verify=expected,noi128 -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=i686-linux-android -std=c11
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=i686-linux-android -std=c11
// RUN: %clang_cc1 %s -verify -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=powerpc64-linux-gnu -std=c11
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=powerpc64-linux-gnu -std=c11
// RUN: %clang_cc1 %s -verify -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=powerpc64-linux-gnu -std=c11 \
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=powerpc64-linux-gnu -std=c11 \
// RUN: -target-cpu pwr7
// RUN: %clang_cc1 %s -verify -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=powerpc64le-linux-gnu -std=c11 \
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=powerpc64le-linux-gnu -std=c11 \
// RUN: -target-cpu pwr8 -DPPC64_PWR8
// RUN: %clang_cc1 %s -verify -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=powerpc64-unknown-aix -std=c11 \
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=powerpc64-unknown-aix -std=c11 \
// RUN: -target-cpu pwr8
// RUN: %clang_cc1 %s -verify -fgnuc-version=4.2.1 -ffreestanding \
-// RUN: -fsyntax-only -triple=powerpc64-unknown-aix -std=c11 \
+// RUN: -Wno-default-const-init-unsafe -fsyntax-only -triple=powerpc64-unknown-aix -std=c11 \
// RUN: -mabi=quadword-atomics -target-cpu pwr8 -DPPC64_PWR8
// Basic parsing/Sema tests for __c11_atomic_*
diff --git a/clang/test/Sema/block-return.c b/clang/test/Sema/block-return.c
index 126fc6f953dea..312ded75b6a81 100644
--- a/clang/test/Sema/block-return.c
+++ b/clang/test/Sema/block-return.c
@@ -126,7 +126,7 @@ void foo7(void)
int (^JJ) (void) = ^{ return j; }; // OK
int (^KK) (void) = ^{ return j+1; }; // OK
- __block const int k;
+ __block const int k = 0;
const int cint = 100;
int (^MM) (void) = ^{ return k; };
diff --git a/clang/test/Sema/builtins-bpf.c b/clang/test/Sema/builtins-bpf.c
index fc540260c91c3..d6e17683d6898 100644
--- a/clang/test/Sema/builtins-bpf.c
+++ b/clang/test/Sema/builtins-bpf.c
@@ -69,7 +69,7 @@ unsigned invalid11(struct s *arg, int info_kind) {
}
unsigned valid12(void) {
- const struct s t;
+ const struct s t = {};
return __builtin_preserve_type_info(t, 0) +
__builtin_preserve_type_info(*(struct s *)0, 1);
}
diff --git a/clang/test/Sema/builtins-elementwise-math.c b/clang/test/Sema/builtins-elementwise-math.c
index 5c54202991a85..b5648a5e5c6e8 100644
--- a/clang/test/Sema/builtins-elementwise-math.c
+++ b/clang/test/Sema/builtins-elementwise-math.c
@@ -88,7 +88,7 @@ void test_builtin_elementwise_add_sat(int i, short s, double d, float4 v, int3 i
_BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
ext = __builtin_elementwise_add_sat(ext, ext);
- const int ci;
+ const int ci = 0;
i = __builtin_elementwise_add_sat(ci, i);
i = __builtin_elementwise_add_sat(i, ci);
i = __builtin_elementwise_add_sat(ci, ci);
@@ -154,7 +154,7 @@ void test_builtin_elementwise_sub_sat(int i, short s, double d, float4 v, int3 i
_BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
ext = __builtin_elementwise_sub_sat(ext, ext);
- const int ci;
+ const int ci = 0;
i = __builtin_elementwise_sub_sat(ci, i);
i = __builtin_elementwise_sub_sat(i, ci);
i = __builtin_elementwise_sub_sat(ci, ci);
@@ -214,7 +214,7 @@ void test_builtin_elementwise_max(int i, short s, double d, float4 v, int3 iv, u
_BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
ext = __builtin_elementwise_max(ext, ext);
- const int ci;
+ const int ci = 0;
i = __builtin_elementwise_max(ci, i);
i = __builtin_elementwise_max(i, ci);
i = __builtin_elementwise_max(ci, ci);
@@ -274,7 +274,7 @@ void test_builtin_elementwise_min(int i, short s, double d, float4 v, int3 iv, u
_BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
ext = __builtin_elementwise_min(ext, ext);
- const int ci;
+ const int ci = 0;
i = __builtin_elementwise_min(ci, i);
i = __builtin_elementwise_min(i, ci);
i = __builtin_elementwise_min(ci, ci);
@@ -1070,7 +1070,7 @@ void test_builtin_elementwise_copysign(int i, short s, double d, float f, float4
ext = __builtin_elementwise_copysign(ext, ext);
// expected-error@-1 {{1st argument must be a scalar or vector of floating-point types (was '_BitInt(32)')}}
- const float cf32;
+ const float cf32 = 0.0f;
f = __builtin_elementwise_copysign(cf32, f);
f = __builtin_elementwise_copysign(f, cf32);
f = __builtin_elementwise_copysign(cf32, f);
diff --git a/clang/test/Sema/builtins-overflow.c b/clang/test/Sema/builtins-overflow.c
index 302489c19e379..9928afbd17c6c 100644
--- a/clang/test/Sema/builtins-overflow.c
+++ b/clang/test/Sema/builtins-overflow.c
@@ -9,7 +9,7 @@ void test(void) {
unsigned r;
const char * c;
float f;
- const unsigned q;
+ const unsigned q = 0;
__builtin_add_overflow(); // expected-error {{too few arguments to function call, expected 3, have 0}}
__builtin_add_overflow(1, 1, 1, 1); // expected-error {{too many arguments to function call, expected 3, have 4}}
diff --git a/clang/test/Sema/enable_if.c b/clang/test/Sema/enable_if.c
index 9d46c71274d69..3ef8310a2fef7 100644
--- a/clang/test/Sema/enable_if.c
+++ b/clang/test/Sema/enable_if.c
@@ -52,7 +52,7 @@ size_t strnlen(const char *s, size_t maxlen) // expected-note {{'strnlen' has be
void test2(const char *s, int i) {
// CHECK: define {{.*}}void @test2
- const char c[123];
+ const char c[123] = { 0 };
strnlen(s, i);
// CHECK: call {{.*}}strnlen_real1
strnlen(s, 999);
diff --git a/clang/test/Sema/implicit-decl.c b/clang/test/Sema/implicit-decl.c
index a3f35222d833c..03ee6cdcb204a 100644
--- a/clang/test/Sema/implicit-decl.c
+++ b/clang/test/Sema/implicit-decl.c
@@ -13,7 +13,7 @@ typedef unsigned char Boolean;
extern int printf(__const char *__restrict __format, ...); // both-note{{'printf' declared here}}
void func(void) {
int32_t *vector[16];
- const char compDesc[16 + 1];
+ const char compDesc[16 + 1] = { 0 };
int32_t compCount = 0;
if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-error {{call to undeclared function '_CFCalendarDecomposeAbsoluteTimeV'; ISO C99 and later do not support implicit function declarations}} \
expected-note {{previous implicit declaration}} \
diff --git a/clang/test/Sema/overloadable.c b/clang/test/Sema/overloadable.c
index 9eecad18064e2..4c6fd0102c59a 100644
--- a/clang/test/Sema/overloadable.c
+++ b/clang/test/Sema/overloadable.c
@@ -155,7 +155,7 @@ void incompatible_pointer_type_conversions() {
}
void dropping_qualifiers_is_incompatible() {
- const char ccharbuf[1];
+ const char ccharbuf[1] = {0};
volatile char vcharbuf[1];
void foo(char *c) __attribute__((overloadable));
diff --git a/clang/test/Sema/sizeless-1.c b/clang/test/Sema/sizeless-1.c
index 9ec884b9f1fda..af96023f4bff5 100644
--- a/clang/test/Sema/sizeless-1.c
+++ b/clang/test/Sema/sizeless-1.c
@@ -94,12 +94,12 @@ void func(int sel) {
svint8_t bad_brace_init_int8_6 = {{local_int8, 0}}; // expected-warning {{too many braces around initializer}}
const svint8_t const_int8 = local_int8; // expected-note {{declared const here}}
- const svint8_t uninit_const_int8;
+ const svint8_t uninit_const_int8; // expected-warning {{default initialization of an object of type 'const svint8_t' (aka 'const __SVInt8_t') leaves the object uninitialized and is incompatible with C++}};
volatile svint8_t volatile_int8;
const volatile svint8_t const_volatile_int8 = local_int8; // expected-note {{declared const here}}
- const volatile svint8_t uninit_const_volatile_int8;
+ const volatile svint8_t uninit_const_volatile_int8; // expected-warning {{default initialization of an object of type 'const volatile svint8_t' (aka 'const volatile __SVInt8_t') leaves the object uninitialized and is incompatible...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
All based on review feedback
|
For the record, #define __smp_load_acquire(p) \
({ \
union { __unqual_scalar_typeof(*p) __val; char __c[1]; } __u; \
typeof(p) __p = (p); \
compiletime_assert_atomic_type(*p); \
kasan_check_read(__p, sizeof(*p)); \
switch (sizeof(*p)) { \
case 1: \
asm volatile ("ldarb %w0, %1" \
: "=r" (*(__u8 *)__u.__c) \
: "Q" (*__p) : "memory"); \
break; \
case 2: \
asm volatile ("ldarh %w0, %1" \
: "=r" (*(__u16 *)__u.__c) \
: "Q" (*__p) : "memory"); \
break; \
case 4: \
asm volatile ("ldar %w0, %1" \
: "=r" (*(__u32 *)__u.__c) \
: "Q" (*__p) : "memory"); \
break; \
case 8: \
asm volatile ("ldar %0, %1" \
: "=r" (*(__u64 *)__u.__c) \
: "Q" (*__p) : "memory"); \
break; \
} \
(typeof(*p))__u.__val; \
})/*
* Check at compile time that something is of a particular type.
* Always evaluates to 1 so you may use it easily in comparisons.
*/
#define typecheck(type,x) \
({ type __dummy; \
typeof(x) __dummy2; \
(void)(&__dummy == &__dummy2); \
1; \
})I also see another instance from a recent change in the crypto code: struct scatter_walk {
/* Must be the first member, see struct skcipher_walk. */
union {
void *const addr;
/* Private API field, do not touch. */
union crypto_no_such_thing *__addr;
};
struct scatterlist *sg;
unsigned int offset;
};
struct skcipher_walk {
union {
/* Virtual address of the source. */
struct {
struct {
const void *const addr;
} virt;
} src;
/* Private field for the API, do not use. */
struct scatter_walk in;
};
unsigned int nbytes;
union {
/* Virtual address of the destination. */
struct {
struct {
void *const addr;
} virt;
} dst;
/* Private field for the API, do not use. */
struct scatter_walk out;
};
unsigned int total;
u8 *page;
u8 *buffer;
u8 *oiv;
void *iv;
unsigned int ivsize;
int flags;
unsigned int blocksize;
unsigned int stride;
unsigned int alignmask;
}; |
Whhhhaaaa? This one: is unfortunate. The diagnostic is expected, but I can see why you'd want to silence it. Does adding
That looks like a correct diagnostic to me, but I admit the code is dense enough I may be missing something. Is there a reason why |
It looks like this is /*
* __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
* non-scalar types unchanged.
*/
/*
* Prefer C11 _Generic for better compile-times and simpler code. Note: 'char'
* is not type-compatible with 'signed char', and we define a separate case.
*/
#define __scalar_type_to_expr_cases(type) \
unsigned type: (unsigned type)0, \
signed type: (signed type)0
#define __unqual_scalar_typeof(x) typeof( \
_Generic((x), \
char: (char)0, \
__scalar_type_to_expr_cases(char), \
__scalar_type_to_expr_cases(short), \
__scalar_type_to_expr_cases(int), \
__scalar_type_to_expr_cases(long), \
__scalar_type_to_expr_cases(long long), \
default: (x)))from include/linux/compiler_types.h. I am not that familiar with
Yes, that appears to work to silence the warning but I have not looked to see if this affects code generation yet. I would assume that it would not because the results of the comparison is always dead but I could see things such as sanitizers messing up those optimizations.
Yes, this code is definitely dense... My basic understanding is that initialization and modification of struct vm_area_struct {
...
/*
* Flags, see mm.h.
* To modify use vm_flags_{init|reset|set|clear|mod} functions.
*/
union {
const vm_flags_t vm_flags;
vm_flags_t __private __vm_flags;
};
...
};
static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
loff_t len)
{
...
struct vm_area_struct pseudo_vma;
...
vma_init(&pseudo_vma, mm);
vm_flags_init(&pseudo_vma, VM_HUGETLB | VM_MAYSHARE | VM_SHARED);
...
}
static inline void vm_flags_init(struct vm_area_struct *vma,
vm_flags_t flags)
{
ACCESS_PRIVATE(vma, __vm_flags) = flags;
}
/* sparse defines __CHECKER__; see Documentation/dev-tools/sparse.rst */
#ifdef __CHECKER__
...
# define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
#else /* __CHECKER__ */
...
# define ACCESS_PRIVATE(p, member) ((p)->member)
The other few instances that I looked at either used only one or two fields in an aggregate (not the |
Er, yes and no are both correct answers. If passed an integer scalar, then it results in an unqualified type. If passed a non-integer scalar, it results in whatever you get from To always strip the qualifier, you'd change the macro slightly: https://godbolt.org/z/vTsofhvoo That forces the rvalue conversion on
This case might be reasonable to handle differently, but I'm on the fence too. There's two cases for structure members, broadly:
In both cases, the code is valid and so the warning is a false positive. In both cases, the code is dangerous and the warning is useful. So I kind of think this is a case where we split the field diagnostic out into its own group. So we'd have |
Yes, this appears to be the case for the 10 or so unique cases that I found in the kernel.
Yes, that seems like a reasonable place to start. I would be happy to test such a change. |
Post-commit review feedback on llvm#137166 raised a concern from the Linux kernel about wanting to silence the new diagnostic when the uninitialized object is a const member of a structure. These members can be initialized later if the containing object is non-const, such as through a call to memset, for example. This splits the diagnostic groups into: -Wc++-compat -Wdefault-const-init -Wdefault-const-init-field -Wdefault-const-init-var -Wdefault-const-init-unsafe -Wdefault-const-init-field-unsafe -Wdefault-const-init-var-unsafe
#137961 should hopefully address your concerns |
Post-commit review feedback on #137166 raised a concern from the Linux kernel about wanting to silence the new diagnostic when the uninitialized object is a const member of a structure. These members can be initialized later if the containing object is non-const, such as through a call to memset, for example. This splits the diagnostic groups into: ``` -Wc++-compat -Wdefault-const-init -Wdefault-const-init-field -Wdefault-const-init-var -Wdefault-const-init-unsafe -Wdefault-const-init-field-unsafe -Wdefault-const-init-var-unsafe ``` --------- Co-authored-by: Mariya Podchishchaeva <[email protected]>
| InGroup<DefaultConstInit>, DefaultIgnore; | ||
| def warn_default_init_const_unsafe : Warning< | ||
| "default initialization of an object of type %0%select{| with const member}1 " | ||
| "leaves the object uninitialized and is incompatible with C++">, |
There was a problem hiding this comment.
@AaronBallman I just came across this diagnostic. Why did we chose to mention the incompatibility with C++? I found it odd because the diagnostic is showing up when building a C source file. If the code was in an inline function in a header (that could be consumed from C++) then I think it would make sense to mention incompatibility with C++ but if it's in a C source file it makes less sense to me. When I'm building C source files I don't typically think (or care) about what would happen if I were to build the source file with C++ instead of C.
Really not a huge deal but I'd like to understand the reasoning and here.
There was a problem hiding this comment.
Good question! I wish I didn't. :-D We don't have a way to know whether a particular warning group is enabled, so there's no way to tell the difference between the user passing no flags and getting the default-enabled -Wdefault-const-init-unsafe, whether they passed -Wdefault-const-init-unsafe explicitly, or whether they passed -Wc++-compat. We just can tell that the warn_default_init_const_unsafe diagnostic itself is/is not ignored at a particular source location. So I don't have a good way to drop/add the "and is incompatible with C++" because I don't have a way to test "did the user explicitly opt in to -Wc++-compat.
However, maybe I can hack around this by adding a new off-by-default fake diagnostic that is never emitted by Clang but is in the -Wc++-compat warning group. If that diagnostic is enabled, it must mean the user passed -Wc++-compat...
There was a problem hiding this comment.
I posted #138266 to try to improve this, thank you for bringing it up!
…` test The test broke because a new diagnostic from upstream started firing and was mixed in with the AST dump. The upstream diagnostic is from: ``` commit 576161c Author: Aaron Ballman <[email protected]> Date: Fri Apr 25 08:21:41 2025 -0400 [C] Warn on uninitialized const objects (llvm#137166) ``` The simplest solution is to surpress the new diagnostic. rdar://150458872
Unlike C++, C allows the definition of an uninitialized
constobject. If the object has static or thread storage duration, it is still zero-initialized, otherwise, the object is left uninitialized. In either case, the code is not compatible with C++.This adds a new diagnostic group,
-Wdefault-const-init-unsafe, which is on by default and diagnoses any definition of aconstobject which remains uninitialized.It also adds another new diagnostic group,
-Wdefault-const-init(which also enabled theunsafevariant) that diagnoses any definition of aconstobject (including ones which are zero-initialized). This diagnostic is off by default.Finally, it adds
-Wdefault-const-initto-Wc++-compat. GCC diagnoses these situations under this flag.Fixes #19297