Skip to content

[clang] Fix -Wuninitialized for values passed by const pointers #147221

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 14, 2025
Merged

[clang] Fix -Wuninitialized for values passed by const pointers #147221

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1e26fe
[clang] Fix -Wuninitialized for values passed by const pointers
igorkudrin Jul 5, 2025
d63b5dc
fixup! libcxx tests
igorkudrin Jul 7, 2025
4ff02a7
fixup! Do not detect passing a value by a constant pointer
igorkudrin Jul 8, 2025
d58b066
Merge branch 'main' into const-ptr-uninitialized
igorkudrin Jul 11, 2025
d3ca8c1
Merge branch 'main' into const-ptr-uninitialized
igorkudrin Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions clang/lib/Analysis/UninitializedValues.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,13 +438,10 @@ void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
return;
}
bool isTrivialBody = hasTrivialBody(CE);
// If a value is passed by const pointer to a function,
// we should not assume that it is initialized by the call, and we
// conservatively do not assume that it is used.
// If a value is passed by const reference to a function,
// it should already be initialized.
for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
I != E; ++I) {
// A value passed by const pointer or reference to a function should already
// be initialized.
for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end(); I != E;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this use CE->arguments() in a range based for loop instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd prefer not to do that to keep the fix as atomic as possible and to avoid concealing the actual behavior change among unrelated NFC changes.

++I) {
if ((*I)->isGLValue()) {
if ((*I)->getType().isConstQualified())
classify((*I), isTrivialBody ? Ignore : ConstRefUse);
Expand All @@ -453,7 +450,7 @@ void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
const auto *UO = dyn_cast<UnaryOperator>(Ex);
if (UO && UO->getOpcode() == UO_AddrOf)
Ex = UO->getSubExpr();
classify(Ex, Ignore);
classify(Ex, Use);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the intent of this code is that if the address of a local variable is passed to a const pointer, that local variable is neither considered used nor initialized. When a local variable is passed directly to a call, no special handling should be required, because an lvalue-to-rvalue conversion will be performed, which will be classified as a use. So I think the correct fix is instead:

Suggested change
if (UO && UO->getOpcode() == UO_AddrOf)
Ex = UO->getSubExpr();
classify(Ex, Ignore);
classify(Ex, Use);
if (UO && UO->getOpcode() == UO_AddrOf) {
classify(UO->getSubExpr(), Ignore);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, rather than treating this case as Ignore, it seems best to treat it instead as ConstRefUse, since passing a local variable by const reference and passing its address by const pointer are analogous and should be treated the same way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your suggestion does not cover the case:

void foo(const int *);
void test() {
  int v;
  foo(&v);
}

Note that gcc catches it: https://godbolt.org/z/TdKG35shz

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That said, rather than treating this case as Ignore, it seems best to treat it instead as ConstRefUse, since passing a local variable by const reference and passing its address by const pointer are analogous and should be treated the same way.

TBH, I don't think we need a separate diagnostic for ConstRefUse while it can be reported as any other use of an uninitialized value. But changing that is not the purpose of this patch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your suggestion does not cover the case:

void foo(const int *);
void test() {
  int v;
  foo(&v);
}

Correct. We intentionally don't warn on that -- a "const reference use" is neither treated as initializing nor as using the local variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. We intentionally don't warn on that -- a "const reference use" is neither treated as initializing nor as using the local variable.

What is the basis of this intention?

Copy link
Collaborator

@zygoloid zygoloid Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Warning on such uses was found to lead to false positives. Such as all the cases you found in libc++'s test suite :)

The point is, for foo(&v), where foo takes a const pointer, we simply don't know what foo does to v. It might read from it, or it might just store a pointer to it somewhere. And -Wuninitialized / -Wsometimes-uninitialized are supposed to have zero false positives. What we do know is that foo does not initialize v, which is why we were marking it as ignore here (because otherwise we would treat a pointer to v as escaping, which conservatively marks it initialized).

Copy link
Collaborator

@zygoloid zygoloid Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With my suggested approach plus changing the kind from Ignore to ConstRefUse, we'd start warning under -Wuninitialized-const-reference (which is a subgroup of -Wuninitialized but should be a subgroup of -Wconditional-uninitialized, but that's a separate bug). That'd be consistent and probably reasonable, other than the warning message and flag being a bit inaccurate for this case, and wouldn't introduce false positives into the other uninitialized warning flags (that are supposed to not have false positives).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the explanation. These two cases, passing an uninitialized value and passing the address of such a value, are indeed different, because the address is defined. I guess that implementing your second suggestion, i.e. to use ConstRefUse instead of Ignore, requires a lot more changes; at least, the warning message needs to be updated to reflect the actual case. I'll prepare a separate PR for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zygoloid ping. I've applied this suggestion in the last update. Could you take a look?

}
}
}
Expand Down
8 changes: 6 additions & 2 deletions clang/test/SemaCXX/uninitialized.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,16 @@ void test_const_ptr() {
int a;
int b; // expected-note {{initialize the variable 'b' to silence this warning}}
foo(&a);
bar(&b);
b = a + b; // expected-warning {{variable 'b' is uninitialized when used here}}
bar(&b); // expected-warning {{variable 'b' is uninitialized when used here}}
b = a + b;
int *ptr; //expected-note {{initialize the variable 'ptr' to silence this warning}}
const int *ptr2;
foo(ptr); // expected-warning {{variable 'ptr' is uninitialized when used here}}
foobar(&ptr2);
int *ptr3; // expected-note {{initialize the variable 'ptr3' to silence this warning}}
const int *ptr4; // expected-note {{initialize the variable 'ptr4' to silence this warning}}
bar(ptr3); // expected-warning {{variable 'ptr3' is uninitialized when used here}}
bar(ptr4); // expected-warning {{variable 'ptr4' is uninitialized when used here}}
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...eam.format/output.streams/ostream.formatted/ostream.inserters.arithmetic/pointer.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ int main(int, char**)
{
testbuf<char> sb1;
std::ostream os1(&sb1);
int n1;
int n1 = 0;
os1 << &n1;
assert(os1.good());
std::string s1(sb1.str());

testbuf<char> sb2;
std::ostream os2(&sb2);
int n2;
int n2 = 0;
os2 << &n2;
assert(os2.good());
std::string s2(sb2.str());
Expand Down
4 changes: 2 additions & 2 deletions ...t/output.streams/ostream.formatted/ostream.inserters.arithmetic/pointer.volatile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class testbuf : public std::basic_streambuf<CharT> {
int main(int, char**) {
testbuf<char> sb1;
std::ostream os1(&sb1);
int n1;
int n1 = 0;
os1 << &n1;
assert(os1.good());
std::string s1 = sb1.str();
Expand All @@ -74,7 +74,7 @@ int main(int, char**) {

testbuf<char> sb3;
std::ostream os3(&sb3);
volatile int n3;
volatile int n3 = 0;
os3 << &n3;
assert(os3.good());
std::string s3 = sb3.str();
Expand Down