[clang][analyzer] Ignore unnamed bitfields in UninitializedObjectChecker

[kr-2003]

Fixes #132001

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

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

Test Results

Testing Time: 221.93s

Total Discovered Tests: 991
  Unsupported      :  16 (1.61%)
  Passed           : 968 (97.68%)
  Expectedly Failed:   7 (0.71%)
[100%] Built target check-clang-analysis

Testcase

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

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

Before Patch

After Patch

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Abhinav Kumar (kr-2003)

Changes

Fixes #132001

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

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

Test Results

Testing Time: 221.93s

Total Discovered Tests: 991
  Unsupported      :  16 (1.61%)
  Passed           : 968 (97.68%)
  Expectedly Failed:   7 (0.71%)
[100%] Built target check-clang-analysis

---

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

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp (+3)

diff --git a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
index 6e1222fedad3e..bf7759975b3ec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -332,6 +332,9 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,
     }
 
     if (isPrimitiveType(T)) {
+      if (I->isUnnamedBitField()) {
+        continue;
+      }
       if (isPrimitiveUninit(V)) {
         if (addFieldToUninits(LocalChain.add(RegularField(FR))))
           ContainsUninitField = true;

[Xazax-hun]

Could you add a test case that fails before your patch?

[kr-2003]

Could you add a test case that fails before your patch?

Sure, here it is

Testcase

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

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

Before Patch

After Patch

[steakhal]

We only accept patches with regressions tests, that ensure that in the future we never accidentally break one thing we had broken in the past. This ensures we are heading in the right direction.

Here are two examples I grabbed from the history of this particular checker: (according to git log/blame)

• 4ff7769
• f0dd101

You can see whenever we have a semantic change (aka. not just a refactor) we almost always have a test demonstrating the changed behavior. Have a look at what test files we usually use for this checker, select the "most suitable place" and extend it with the new test case. Remember to add // expected-warning {{...}} comment where relevant to mandate some output.
We usually also put a no-warning comment at some places where we had a diagnostic in the past and we have just fixed it. (This is your case).

[steakhal]

@YLChenZ had a slightly different solution. I redirected him here to challenge your proposal.

[YLChenZ]

@steakhal Okay, glad to do it. But I don't know specifically about the workflow of review. Could you give me some guidance?

[steakhal]

@steakhal Okay, glad to do it. But I don't know specifically about the workflow of review. Could you give me some guidance?

How is this proposal different from yours?
What do you think is objectively a better approach?
Do the added tests cover the intended behavior?
Would the test indeed fail without the fix, but pass with the fix?
Which approach is more maintainable for the long run?
Can we think of some other solutions that we haven't discussed yet?

[YLChenZ]

My proposal is to judge the current FieldDecl at the beginning of the loop, and if it's a UnamedBitField, just skip it, because at that point the UnamedBitField's static check should be passing. If it's a NamedBitField then it needs to be initialized to pass the static check (i.e. go deeper to determine the type, value or whatever).

for (const FieldDecl *I : RD->fields()) {
    if (I->isUnnamedBitField()) {   //my proposal 
      continue;                              
    }
    ......
    if (isPrimitiveType(T)) {
        // Skip unnamed bitfields (padding)
        if (I->isUnnamedBitField()) {   //his proposal 
            continue;
        }
        ......
    }
}

The current test cases are sufficient. I'm not sure about the answers to the other questions.

[steakhal]

My proposal is to judge the current FieldDecl at the beginning of the loop, and if it's a UnamedBitField, just skip it, because at that point the UnamedBitField's static check should be passing. If it's a NamedBitField then it needs to be initialized to pass the static check (i.e. go deeper to determine the type, value or whatever).

Exactly. I think it makes a lot more sense to check this as early as possible to have a reduced set of possibilities to think about later. @kr-2003 Could you please update your PR accordingly?

The current test cases are sufficient. I'm not sure about the answers to the other questions.

I agree.

[kr-2003]

My proposal is to judge the current FieldDecl at the beginning of the loop, and if it's a UnamedBitField, just skip it, because at that point the UnamedBitField's static check should be passing. If it's a NamedBitField then it needs to be initialized to pass the static check (i.e. go deeper to determine the type, value or whatever).

Exactly. I think it makes a lot more sense to check this as early as possible to have a reduced set of possibilities to think about later. @kr-2003 Could you please update your PR accordingly?

The current test cases are sufficient. I'm not sure about the answers to the other questions.

I agree.

Done

[steakhal]

LGTM

[github-actions]

@kr-2003 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!