clang-analyzer-optin.cplusplus.UninitializedObject false positive with unnamed fields

[firewave]

struct S
{
    S(bool b)
    : b(b)
    {}
    bool b{false};
    long long : 7; // padding
};

void f()
{
    S s(true);
}

<source>:4:9: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
    4 |     : b(b)
      |         ^
<source>:7:15: note: uninitialized field 'this->'
    7 |     long long : 7; // padding
      |               ^
<source>:12:7: note: Calling constructor for 'S'
   12 |     S s(true);
      |       ^~~~~~~
<source>:4:9: note: 1 uninitialized field at the end of the constructor call
    4 |     : b(b)
      |         ^

https://godbolt.org/z/7zzoK97x5

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Oliver Stöneberg (firewave)

```cpp struct S { S(bool b) : b(b) {} bool b{false}; long long : 7; // padding };

void f()
{
S s(true);
}

<source>:4:9: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
4 | : b(b)
| ^
<source>:7:15: note: uninitialized field 'this->'
7 | long long : 7; // padding
| ^
<source>:12:7: note: Calling constructor for 'S'
12 | S s(true);
| ^~~~~~~
<source>:4:9: note: 1 uninitialized field at the end of the constructor call
4 | : b(b)
| ^

https://godbolt.org/z/7zzoK97x5
</details>

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under clang/test/Analysis create fine-grained testing targets, so you can e.g. use make check-clang-analysis to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Oliver Stöneberg (firewave)

```cpp struct S { S(bool b) : b(b) {} bool b{false}; long long : 7; // padding };

void f()
{
S s(true);
}

<source>:4:9: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
4 | : b(b)
| ^
<source>:7:15: note: uninitialized field 'this->'
7 | long long : 7; // padding
| ^
<source>:12:7: note: Calling constructor for 'S'
12 | S s(true);
| ^~~~~~~
<source>:4:9: note: 1 uninitialized field at the end of the constructor call
4 | : b(b)
| ^

https://godbolt.org/z/7zzoK97x5
</details>

[steakhal]

Probably somewhere in https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp we should check if the uninitialized field is an unnamed bitfield and bail out.

[YLChenZ]

@steakhal hey can you provide a bit more info on this issue and assign me I would like to work on it

[steakhal]

@steakhal hey can you provide a bit more info on this issue and assign me I would like to work on it

Hey, nice to see you interested. I'm not going to assign the issue, but it shouldn't matter. Feel free to work on this.

So, the file that needs to be looked at is clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp, and probably the constructor of FindUninitializedFields kicks off the checking, so probably it should check if the FieldDecl is a bitfield - by checking the FieldDecl::isBitField member function. In that case, probably the checker should bail out and NOT report the field as uninitialized.

I don't know the actual implementation of this, but it shouldn't be difficult to follow. I trust you here.
Once you have a grip on it, you should prove the fix in a test. Have a look at git blame, and git log of this file to see example commits. We always have tests for semantic changes, so you will see some examples of how testing it done.

To run the tests, you can use the check-clang-analysis build target or directly use the llvm-lit. For example:

ninja -C build check-clang-analysis

Or

./build/bin/llvm-lit -sv clang/test/Analysis/fancytest.cpp

Once you have something, we can continue the discussion under a draft PR, at which point I'll assign this issue for you.
If you have other issues, of course, don't hesitate to ask here.

[kr-2003]

@steakhal Here, are some of my findings and probable solution:
1.

* frame #0: 0x00000001079a8db8 libclang-cpp.dylib`(anonymous namespace)::RegularField::printNoteMsg(llvm::raw_ostream&) const
    frame #1: 0x00000001079a8134 libclang-cpp.dylib`clang::ento::FieldChainInfo::printNoteMsg(llvm::raw_ostream&) const + 52
    frame #2: 0x00000001079a5e58 libclang-cpp.dylib`clang::ento::FindUninitializedFields::addFieldToUninits(clang::ento::FieldChainInfo, clang::ento::MemRegion const*) + 728
    frame #3: 0x00000001079a5658 libclang-cpp.dylib`clang::ento::FindUninitializedFields::isNonUnionUninit(clang::ento::TypedValueRegion const*, clang::ento::FieldChainInfo) + 968
    frame #4: 0x00000001079a51ec libclang-cpp.dylib`clang::ento::FindUninitializedFields::FindUninitializedFields(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>, clang::ento::TypedValueRegion const*, clang::ento::UninitObjCheckerOptions const&) + 228

This is the backtrace while running

lldb -- /opt/homebrew/opt/llvm/bin/clang -cc1 -analyze -analyzer-checker=optin.cplusplus.UninitializedObject test.cpp

2. Execution goes from FindUninitializedFields::FindUninitializedFields constructor to FindUninitializedFields::addFieldToUninits function.
3. Inside FindUninitializedFields::addFieldToUninits function, it falls under this block:

    if (isPrimitiveType(T)) {
      if (isPrimitiveUninit(V)) {
        if (addFieldToUninits(LocalChain.add(RegularField(FR))))
          ContainsUninitField = true;
      }
      continue;
    }

4. Edit isNonUnionUninit (caller of isPrimitiveUninit): Add a check before calling isPrimitiveUninit

if (isPrimitiveType(T)) {
    // Skip unnamed bitfields (padding)
    if (I->isBitField() && I->isUnnamedBitfield()) {
        IsAnyFieldInitialized = true; // Treat as initialized to avoid warning
        continue;
    }
    if (isPrimitiveUninit(V)) {
        if (addFieldToUninits(LocalChain.add(RegularField(FR))))
            ContainsUninitField = true;
    }
    continue;
}

[steakhal]

IDK the full context, but what you suggest matches my initial feeling. And this is why I tagged this with good first issue.
I think the isUnnamedBitfield() already checks if it's a bitfield, so probably you wouldn't even need a conjunction there.

I'm not sure about setting IsAnyFieldInitialized though. It depends on the semantic of that flag. If it represents that we saw at least one explicit initialization of a field, then we shouldn't set it for example.

[YLChenZ]

@steakhal
My solution is to directly determine if the current field declaration is an unnamed bitfield at the beginning of the current loop, like this

for (const FieldDecl *I : RD->fields()) {
    if (I->isUnnamedBitField()) {
      IsAnyFieldInitialized = true;
      continue;
    }
    ......
}

At this point, we only care about whether the current field is unnamed or not, and do not go deeper to determine whether the type of the field is legal or not, because this has already been done in isUnnamedBitField().

Here are my test results：

Total Discovered Tests: 989
Unsupported : 19 (1.92%)
Passed : 964 (97.47%)
Expectedly Failed: 6 (0.61%)

[steakhal]

Please submit a PR, and continue the discussion there.
One who opens a PR will receive a review from the other interested contributors (including people who may not contributed to the project).
Once we all reach an alignment there, I'll make sure the right co-authors set for the PR as attribution.