[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion updated this revision to Diff 505802.
Tedlion edited the summary of this revision.
Tedlion added a comment.

Provide the full diff context instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146194/new/

https://reviews.llvm.org/D146194

Files:
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 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  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 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  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion accepted this revision.
Tedlion added a comment.

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

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  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
if (V.isUndef())
  return true;
  }
  FieldChain.pop_back();
}
  
  ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146194/new/

https://reviews.llvm.org/D146194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion requested review of this revision.
Tedlion added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146194/new/

https://reviews.llvm.org/D146194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion created this revision.
Tedlion added a reviewer: clang.
Tedlion added a project: clang.
Herald added subscribers: steakhal, martong.
Herald added a reviewer: NoQ.
Herald added a project: All.
Tedlion requested review of this revision.
Herald added a subscriber: cfe-commits.

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 :

  // 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);
  }
  }




Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146194

Files:
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 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  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 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  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits