NOT report the warning that unnamed bitfields are uninitialized.

[Tedlion]

Summary

I found current clang reports a warning

warning: Passed-by-value struct argument contains uninitialized data (e.g., field: '') [core.CallAndMessage]

when do static analysis on the following code (processed as C source file) :

// code piece 1
typedef struct{
    unsigned x :3;
    unsigned   :29; 
    unsigned y;
}A;

extern void func1(A a);

void func2(){
    A a = {0};
    func1(a);
}
}

According C or C++ standards, "unnamed bit-fields are skipped during aggregate initialization." Unnamed bit-fields can not be explicitly initialized, and taking no explicitly initialization will not cause problems since they would not be used.

With this patch, no warning will be reported on the above code. And the checker will still be effective to real warning. e.g. it will report " warning: Passed-by-value struct argument contains uninitialized data (e.g., field: 'y') [core.CallAndMessage]" on following code:

typedef struct{
    unsigned x :3;
    unsigned   :29;
    unsigned y;
}A;

extern void func1(A a);

void func2(){
    A a;
    a.x = 0;
    func1(a);
}
}

[steakhal]

Thanks for looking into this.

I could not reproduce the issue with the following test code: https://godbolt.org/z/hsY9PTKGz

struct A {
  unsigned x : 3;
  unsigned : 29; 
  unsigned y;
};
void func1(A a);

void func2() {
  A a = {0};
  func1(a); // no-warning here on trunk
}

I'd recommend adding a test that would demonstrate that before your fix the test would diagnose a FP and after your fix, the FP would disappear.
Without tests, unfortunately, we cannot accept patches.

[Tedlion]

In D146194#4198870, @steakhal wrote:

Thanks for looking into this.

I could not reproduce the issue with the following test code: https://godbolt.org/z/hsY9PTKGz

struct A {
  unsigned x : 3;
  unsigned : 29; 
  unsigned y;
};
void func1(A a);

void func2() {
  A a = {0};
  func1(a); // no-warning here on trunk
}

I'd recommend adding a test that would demonstrate that before your fix the test would diagnose a FP and after your fix, the FP would disappear.
Without tests, unfortunately, we cannot accept patches.

Thanks for reviewing. I have tried the code as C++ source, no warning is reported. However, clang gives the wrong warning when processing the code as C source. Try the following: https://godbolt.org/z/6ET6rnb4z

[steakhal]

Okay, thanks for clarifying.

So, it seems like it depends on the language flavor, but why does CSA behave differently in C and C++?
This is necessary to understand if we need to fix that instead of patching this particular checker.

I'd still add a test to the clang/test/Analysis/unnamedBitfields.c, something like this:

// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s -x c
// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s -x c++

typedef struct {
  unsigned x : 3;
  unsigned : 29; 
  unsigned y;
} A;
void take_by_value(A a);

void initlistExpr(void) {
  A a = {0}; // zero-initializes all fields
  take_by_value(a); // no-warning
}

void defaultConstruct(void) {
  A a; // uninitialized
  a.x = 0;
  take_by_value(a); // expected-warning{{Passed-by-value struct argument contains uninitialized data (e.g., field: 'y')}}
}

---

FYI you can add syntax highlighting for your snippets here by:

```lang=C++

[steakhal]

BTW we request full context for git patches. That way the reviewer can scroll and see the context around your hunks.
You can request git to emit the full context by passing the -U999999 commandline option. See.

As per your proposed change, I think we should do something like this:

for (const auto *I : RD->fields()) {
  if (I->isUnnamedBitfield())
    continue;

instead of embedding this into some condition.

But I'm curious to see why does C behave differently.

[Tedlion]

I argee that the different CSA behavior between C and C++ should be checked.

Sorry, I do not fully accept the revision but took wrong action.

In D146194#4199257, @steakhal wrote:

As per your proposed change, I think we should do something like this:

for (const auto *I : RD->fields()) {
  if (I->isUnnamedBitfield())
    continue;

instead of embedding this into some condition.

Note in the begin and end of the for loop body, there is a push_back and pop_back action to FieldChain, that's why I have to emembed the new condition into the old. (OK now I know large diff context is necessary.)

...
      for (const auto *I : RD->fields()) {
        const FieldRegion *FR = MrMgr.getFieldRegion(I, R);
        FieldChain.push_back(I);    
        T = I->getType();
        if (T->getAsStructureType()) {
          if (Find(FR))
            return true;
        } else {
          const SVal &V = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
          if (V.isUndef())
            return true;
        }
        FieldChain.pop_back();
      }

...

[Tedlion]

Provide the full diff context instead.

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

if (const RecordType *RT = T->getAsStructureType()) {
       assert(RD && "Referred record has no definition");
       for (const auto *I : RD->fields()) {
         const FieldRegion *FR = MrMgr.getFieldRegion(I, R);
         FieldChain.push_back(I);
         T = I->getType();
         if (T->getAsStructureType()) {
           if (Find(FR))
             return true;
-        } else {
+        } else if (!I->isUnnamedBitfield()){
+          // unnamed bitfields are skipped during aggregate initialization
+          // Since they are not supposed to be used, warnings should not be reported
           const SVal &V = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
           if (V.isUndef())
             return true;
         }
         FieldChain.pop_back();
       }
     }