[clang][dataflow] Crashes in getChild with incorrect "base" StorageLocation after #127338

[jvoung]

After #127338 we are seeing some crashes in bugprone-unchecked-optional-access checker in the dataflow framework.

New test to trigger the crash: #128065
inline:

   struct NonTrivDtor {
      NonTrivDtor(int x);
      ~NonTrivDtor() {}
    };
    struct Other {
      std::optional<int> x = std::nullopt;
      NonTrivDtor y = x.has_value() ? NonTrivDtor(*x) : NonTrivDtor(-1);
    };
    struct target {
      target() { Other{}; }
    };

1. evaluating the ListInit for Other{} in the target ctor
2. involves some default inits for y, which references this->x -- the this should be for Other
3. but the this is being confused for target (also in target's ctor)

Example stack:

 #0 0x000056125e5df14b llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (clang/unittests/Analysis/FlowSensitive
/./ClangAnalysisFlowSensitiveTests+0x53a14b)
 #1 0x000056125e5dbe16 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
 #2 0x00007f383c574590 (/lib/x86_64-linux-gnu/libc.so.6+0x3f590)
 #3 0x00007f383c5c33ac __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #4 0x00007f383c5744f2 raise ./signal/../sysdeps/posix/raise.c:27:6
 #5 0x00007f383c55d4ed abort ./stdlib/abort.c:81:7
 #6 0x00007f383c55d415 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #7 0x00007f383c56d012 (/lib/x86_64-linux-gnu/libc.so.6+0x38012)
 #8 0x000056125e3d64b2 clang::dataflow::RecordStorageLocation::getChild(clang::ValueDecl const&) const (clang/unit
tests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x3314b2)
 #9 0x000056125ed61b55 clang::dataflow::(anonymous namespace)::TransferVisitor::VisitMemberExpr(clang::MemberExpr const*) Transfer.cpp:0:0
#10 0x000056125ed64b9a clang::dataflow::transfer(clang::dataflow::StmtToEnvMap const&, clang::Stmt const&, clang::dataflow::Environment&, clang::dataflow::Environment::ValueModel&) (/
usr/local/google/home/jvoung/w/jvoung-llvm/llvm-project/build/tools/clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0xcbfb9a)
#11 0x000056125ed54a30 clang::dataflow::transferCFGBlock(clang::CFGBlock const&, clang::dataflow::(anonymous namespace)::AnalysisContext&, clang::dataflow::CFGEltCallbacksTypeErased c
onst&) TypeErasedDataflowAnalysis.cpp:0:0
#12 0x000056125ed55a26 clang::dataflow::runTypeErasedDataflowAnalysis(clang::dataflow::AdornedCFG const&, clang::dataflow::TypeErasedDataflowAnalysis&, clang::dataflow::Environment co
nst&, clang::dataflow::CFGEltCallbacksTypeErased const&, int) (clang/unittests/Analysis/FlowSensitive/./ClangAnaly
sisFlowSensitiveTests+0xcb0a26)
#13 0x000056125e5568d2 llvm::Error clang::dataflow::test::checkDataflow<clang::dataflow::UncheckedOptionalAccessModel>(clang::dataflow::test::AnalysisInputs<clang::dataflow::UncheckedOptionalAccessModel>, std::function<void (clang::dataflow::test::AnalysisOutputs const&)>) (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x4b18d2)
#14 0x000056125e5573d1 llvm::Error clang::dataflow::test::checkDataflow<clang::dataflow::UncheckedOptionalAccessModel>(clang::dataflow::test::AnalysisInputs<clang::dataflow::UncheckedOptionalAccessModel>, std::function<void (llvm::DenseMap<unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, llvm::DenseMapInfo<unsigned int, void>, llvm::detail::DenseMapPair<unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, clang::dataflow::test::AnalysisOutputs const&)>) (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x4b23d1)
#15 0x000056125e55a2dd void UncheckedOptionalAccessTest::ExpectDiagnosticsFor<clang::ast_matchers::internal::Matcher<clang::NamedDecl>>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, clang::ast_matchers::internal::Matcher<clang::NamedDecl>, char const*, bool) (.isra.0) UncheckedOptionalAccessModelTest.cpp:0:0
#16 0x000056125e55a977 UncheckedOptionalAccessTest::ExpectDiagnosticsFor(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool) (.constprop.0.isra.0) UncheckedOptionalAccessModelTest.cpp:0:0
#17 0x000056125e55aa4c UncheckedOptionalAccessTest_ConstructorOtherStructField_Test::TestBody() (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x4b5a4c)
#18 0x000056125e63789e testing::Test::Run() (.part.0) gtest-all.cc:0:0
#19 0x000056125e63ecf2 testing::TestInfo::Run() (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x599cf2)
#20 0x000056125e64b2f9 testing::TestSuite::Run() (.part.0) gtest-all.cc:0:0
#21 0x000056125e64bcba testing::internal::UnitTestImpl::RunAllTests() (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x5a6cba)
#22 0x000056125e64c54e testing::UnitTest::Run() (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x5a754e)
#23 0x000056125e33eb09 main (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x299b09)
#24 0x00007f383c55ec8a __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#25 0x00007f383c55ed45 call_init ./csu/../csu/libc-start.c:128:20
#26 0x00007f383c55ed45 __libc_start_main ./csu/../csu/libc-start.c:347:5
#27 0x000056125e3888d1 _start (clang/unittests/Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests+0x2e38d1)

[yronglin]

@jvoung Thank you report this issue, can you provide a simple reproducer for this issue? I'm sorry that I'm not familier with clang-tidy, I tried to reproduce it locally but failed(https://godbolt.org/z/7331q8cb5).

[jvoung]

Sure, I think you were mainly missing the clang-tidy tool window and args -checks='-*,bugprone-unchecked-optional-access' in compiler explorer: https://godbolt.org/z/YTGa9YGh4

Otherwise the test from a7002fb can be run with check-clang (or maybe check-clang-tools)

[yronglin]

Hmmm, it seems we need to support default-init/-arg in dataflow analysis, the CFG looks very different:

A more simple reproducer:

namespace std {
template <typename T> struct optional {
  bool has_value() const;
  T &operator*();
};
constexpr optional<int> nullopt;
} // namespace std

struct NonTrivDtor {
  NonTrivDtor(int x);
  ~NonTrivDtor() {}
};
struct Other {
  std::optional<int> x = std::nullopt;
  NonTrivDtor y = x.has_value() ? NonTrivDtor(*x) : NonTrivDtor(-1);
};
struct target {
  target() { Other{}; }
};

Before #127338

 [B4 (ENTRY)]
   Succs (1): B3

 [B1]
   1: ~NonTrivDtor() (Temporary object destructor)
   Preds (1): B3
   Succs (1): B0

 [B2]
   1: ~NonTrivDtor() (Temporary object destructor)
   Preds (1): B3
   Succs (1): B0

 [B3]
   1: 
   2: 
   3: {}
   4: [B3.3] (BindTemporary)
   5: Other[B3.4] (CXXFunctionalCastExpr, NoOp, Other)
   6: ~Other() (Temporary object destructor)
   T: (Temp Dtor) -1
   Preds (1): B4
   Succs (2): B2 B1

 [B0 (EXIT)]
   Preds (2): B1 B2

After:

 [B7 (ENTRY)]
   Succs (1): B6

 [B1]
   1: ~NonTrivDtor() (Temporary object destructor)
   Preds (1): B3
   Succs (1): B0

 [B2]
   1: ~NonTrivDtor() (Temporary object destructor)
   Preds (1): B3
   Succs (1): B0

 [B3]
   1: [B6.6] ? [B4.9] : [B5.5]
   2: [B3.1]
   3: 
   4: {}
   5: [B3.4] (BindTemporary)
   6: Other[B3.5] (CXXFunctionalCastExpr, NoOp, Other)
   7: ~Other() (Temporary object destructor)
   T: (Temp Dtor) [B5.4]
   Preds (2): B4 B5
   Succs (2): B2 B1

 [B4]
   1: operator*
   2: [B4.1] (ImplicitCastExpr, FunctionToPointerDecay, int &(*)(void))
   3: this
   4: [B4.3]->x
   5: * [B4.4] (OperatorCall)
   6: [B4.5] (ImplicitCastExpr, LValueToRValue, int)
   7: [B4.6] (CXXConstructExpr, NonTrivDtor)
   8: [B4.7] (BindTemporary)
   9: NonTrivDtor([B4.8]) (CXXFunctionalCastExpr, ConstructorConversion, NonTrivDtor)
   Preds (1): B6
   Succs (1): B3

 [B5]
   1: 1
   2: -[B5.1]
   3: [B5.2] (CXXConstructExpr, NonTrivDtor)
   4: [B5.3] (BindTemporary)
   5: NonTrivDtor([B5.4]) (CXXFunctionalCastExpr, ConstructorConversion, NonTrivDtor)
   Preds (1): B6
   Succs (1): B3

 [B6]
   1: 
   2: this
   3: [B6.2]->x
   4: [B6.3] (ImplicitCastExpr, NoOp, const struct std::optional<int>)
   5: [B6.4].has_value
   6: [B6.5]()
   T: [B6.6] ? ... : ...
   Preds (1): B7
   Succs (2): B4 B5

 [B0 (EXIT)]
   Preds (2): B1 B2

[yronglin]

CC @steakhal

[steakhal]

I'm not familiar with the clang DFG. /cc @Xazax-hun for an expert there.
I'll never the less pay attention to this issue to maybe learn something :)

[Xazax-hun]

involves some default inits for y, which references this->x -- the this should be for Other
but the this is being confused for target (also in target's ctor)

Do you mean that the CFG has the wrong this expression in the basic block (with the wrong type)? Or is this a confusion in the dataflow analysis framework? Unfortunately, the dump alone does not answer the question.

[Xazax-hun]

I haven't looked into this deeply, but I think some codepaths might have the expectation that CXXThisExpr always refers to the parent type of the current member function. With default arg/init rewrite this is no longer true. The injected initializer expressions might have CXXThisExprs that actually refer to the currently initialized object rather than the enclosing type of the member function.