[Static Analyzer] Structured bindings to data members

[isuckatcs]

Summary

Handling structured bindings to data members are relatively straightforward, as everything is taken care of by handleConstructor.

All that's left to do is just reading the values from the specific region.

At the moment if the struct is not considered a small struct (for more information see RegionStoreManager::bindStruct()), a lazy
compound value is created insted of an actual copy, from which the static analyzer fails to read the field values if they are Undefined,
it reads Unknown instead.

[martong]

I am not sure in what maturity do we handle lambda captures, but if we do handle them then this https://reviews.llvm.org/D122768 should be of your and your mentor's interest.

[NoQ]

clang/test/Analysis/uninit-structured-binding-struct.cpp

69

Another thing we could try to test is whether i is known to be equal to 1:

clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}

(https://clang.llvm.org/docs/analyzer/developer-docs/DebugChecks.html#exprinspection-checks)

[NoQ]

I think this patch is straightforward and good to go. It's nice to know that we already handle most of this stuff out of the box.

In D127643#3578265, @martong wrote:

I am not sure in what maturity do we handle lambda captures, but if we do handle them then this https://reviews.llvm.org/D122768 should be of your and your mentor's interest.

Oh thanks, that's gonna be fun! Our handling of lambdas is more or less ok, it still has missing pieces but we can at least handle some cases.

[isuckatcs]

Addressed the comment about using clang_analyzer_eval()

[xazax.hun]

I think you might want to add some test cases with a non-pod class. Of course, we can do way less in those cases. But at least the tests would demonstrate that we do not crash on code like that.

Otherwise it looks good to me, nice work!

[xazax.hun]

clang/test/Analysis/uninit-structured-binding-struct.cpp

49

Maybe we could also test the following scenario:

i = 2;
clang_analyzer_eval(tst.a == 2);

[isuckatcs]

Addressed comments

[xazax.hun]

I think we stall lack a non-POD testcase. Otherwise it is good to go :)

[isuckatcs]

• Updated existing testcases so clang_analyzer_eval() evalutes the arguments
• Added testcases with non-POD types.

[xazax.hun]

Thanks, I think this is now OK to commit.

[steakhal]

I missed this commit because it was not tagged by "[analyzer]".
What is the community preference about tagging CSA commits?

[Szelethus]

I read https://en.cppreference.com/w/cpp/language/structured_binding carefully, and there are a number of interesting rules that might deserve their own test case, even if this isn't the patch where you solve that issue, or believe that the solution handles it without the need for special case handling.

Just to name a few, you seem to not have test cases for when the bindings are cv-qualified. If you declare structured bindings like this:

struct s {
  int a;
  int b;
};

void a(void) {
  s tst;

  auto [i, j](tst); // Syntax (3) according to cppreference

  int x = i; // expected-warning{{Assigned value is garbage or undefined}}
}

then the bindings are direct initialized, not copy initialized.

I'm not sure that the actual standard has anything that cppreference doesn't, but maybe we could give it a glance.

clang/test/Analysis/uninit-structured-binding-struct.cpp

11

In the long run, it might make your own life easier to create more talkative test function names, like testPODDecomp or smt like that.

[NoQ]

In D127643#3595731, @steakhal wrote:

I missed this commit because it was not tagged by "[analyzer]".
What is the community preference about tagging CSA commits?

I think [analyzer] is a good tag, no need to change anything. I also just set up my subscriptions by directories rather than by tags and I like how it works.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

Bldr.generateNode(Ex, Pred, state->BindExpr(Ex, LCtx, V), nullptr,
                       ProgramPoint::PostLValueKind);
     return;
   }
   if (isa<FieldDecl, IndirectFieldDecl>(D)) {
     // Delegate all work related to pointer to members to the surrounding
     // operator&.
     return;
   }
-  if (isa<BindingDecl>(D)) {
-    // FIXME: proper support for bound declarations.
-    // For now, let's just prevent crashing.
+  if (const auto *BD = dyn_cast<BindingDecl>(D)) {
+    const auto *DD = cast<DecompositionDecl>(BD->getDecomposedDecl());
+
+    if (const auto *ME = dyn_cast<MemberExpr>(BD->getBinding())) {
+      const auto *Field = cast<FieldDecl>(ME->getMemberDecl());
+
+      SVal Base = state->getLValue(DD, LCtx);
+      if (DD->getType()->isReferenceType()) {
+        Base = state->getSVal(Base.getAsRegion());
+      }
+
+      SVal V = state->getLValue(Field, Base);
+
+      Bldr.generateNode(Ex, Pred, state->BindExpr(Ex, LCtx, V));
+    }
+
     return;
   }
 
   llvm_unreachable("Support for this Decl not implemented.");
 }
 
 /// VisitArraySubscriptExpr - Transfer function for array accesses
 void ExprEngine::VisitArraySubscriptExpr(const ArraySubscriptExpr *A,

clang/test/Analysis/uninit-structured-binding-struct.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct s {
+  int a;
+  int b;
+};
+
+void a(void) {
+  s tst;
+
+  auto [i, j] = tst;
+
+  int x = i; // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+void b(void) {
+  s tst;
+  tst.a = 1;
+
+  auto [i, j] = tst;
+
+  clang_analyzer_eval(i == 1); // expected-warning{{TRUE}}
+  int y = j;                   // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+void c(void) {
+  s tst;
+
+  auto &[i, j] = tst;
+
+  int x = i; // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+void d(void) {
+  s tst;
+  tst.a = 1;
+
+  auto &[i, j] = tst;
+
+  clang_analyzer_eval(i == 1); // expected-warning{{TRUE}}
+  i = 2;
+  clang_analyzer_eval(tst.a == 2); // expected-warning{{TRUE}}
+
+  int y = j; // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+void e(void) {
+  s tst;
+  tst.a = 1;
+
+  auto &[i, j] = tst;
+
+  clang_analyzer_eval(i == 1); // expected-warning{{TRUE}}
+
+  tst.b = 2;
+  clang_analyzer_eval(j == 2); // expected-warning{{TRUE}}
+}
+
+void f(void) {
+  s tst;
+
+  auto &&[i, j] = tst;
+
+  int x = i; // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+void g(void) {
+  s tst;
+  tst.a = 1;
+
+  auto &&[i, j] = tst;
+
+  clang_analyzer_eval(i == 1); // expected-warning{{TRUE}}
+  int y = j;                   // expected-warning{{Assigned value is garbage or undefined}}
+}
+
+struct s2 {
+  int a = 1;
+  int b = 2;
+};
+
+struct s3 {
+  s x;
+  s2 y;
+};
+
+void h(void) {
+  s3 tst;
+
+  clang_analyzer_eval(tst.y.a == 1); // expected-warning{{TRUE}}
+
+  auto [i, j] = tst;
+
+  // FIXME: These should be undefined, but we have to fix
+  // reading undefined from lazy compound values first.
+  clang_analyzer_eval(i.a); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(i.b); // expected-warning{{UNKNOWN}}
+
+  clang_analyzer_eval(j.a == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j.b == 2); // expected-warning{{TRUE}}
+}
+
+void i(void) {
+  s3 tst;
+
+  clang_analyzer_eval(tst.y.a == 1); // expected-warning{{TRUE}}
+
+  auto &[i, j] = tst;
+  j.a = 3;
+
+  clang_analyzer_eval(tst.y.a == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(tst.y.b == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j.b == 2);     // expected-warning{{TRUE}}
+}