This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add support for CXXInheritedCtorInitExpr.
ClosedPublic

Authored by NoQ on Feb 17 2020, 11:18 AM.

Details

Summary

CXXInheritedCtorInitExpr shows up when code looks roughly like this:

struct A {
  A(int x) { ... }
};

struct B {
  using A::A;  // !!
};

The AST for the autogenerated constructor for B looks like this:

`-CXXConstructorDecl implicit used A 'void (int)' inline
  |-ParmVarDecl implicit 'int'
  |-CXXCtorInitializer 'A'
  | `-CXXInheritedCtorInitExpr 'A'  // !!
  `-CompoundStmt

So far we've been dropping coverage every time we've encountered CXXInheritedCtorInitExpr. This patch attempts to add some support for it.

The AST is rather annoying to work with. CXXInheritedCtorInitExpr is not related to CXXConstructExpr, so it requires a new CallEvent sub-class in order to capture its semantics properly.

There are also no argument expressions, even though the inherited constructor for A takes one argument x. The current patch takes argument expressions from the topmost CXXConstructExpr, which also requires a different location context to access. As far as i can see, the result of such substitution is always correct, i.e., the resulting argument SVal obtained from the Environment this way will always be evalBinOp-equal to the correct argument value. I'm still not sure it's a great long-term solution; it might be better to outright forbid calling getArgExpr() and getNumArgs() on a CXXInheritedConstructorCall.

Another piece of work that needs to be done (probably in follow-up patches) is supporting argument constructors in the CXXInheritedCtorInitExpr; the last test shows that it's not quite working. It's also going to be very annoying because the construct-expressions for such arguments are also not present in the AST. So this is going to be yet another kind of CallEvents that don't correspond to any Expr (the previous such case being destructors that aren't invoked manually).

Diff Detail

Event Timeline

NoQ created this revision.Feb 17 2020, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 11:18 AM
martong added inline comments.Feb 18 2020, 2:51 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
891

Perhaps the example could provide the definition of the class S too.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
436

Should there be rather CIE in the assert? Or the text after && is confusing.

497

CIE ?

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
545

live -> alive ?

If the AST is hard to work with would it make sense to try to change the AST a bit?

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
942

Maybe reusing getCXXThisVal here to reduce the number of casts?

If the AST is hard to work with would it make sense to try to change the AST a bit?

Hmm, you mean making CXXConstructExpr an abstract base with subclasses CXXSimpleConstructExpr and CXXInheritedConstructExpr? For me it seems reasonable but I am afraid it would be a huge impact to clang, and not only to the analyzer.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
915

I wonder whether we could reduce code-duplication using some kind of mixin-like inheritance here.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
436

Surely not CIE, since the code below uses CE. I see nothing confusing here: CE must exist because complete constructors cannot be inherited, thus CIE cannot exist.

497

No. CE. Since inherited constructors do not have construction contexts, State is the same for CIE as the previous State. Thus if they are different, we are facing a CE.

clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
31

Not constructors_with_arguments?

martong added inline comments.Feb 18 2020, 8:06 AM
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
436

Maybe it's just me and seems like nit picking, but this is still confusing for me b/c above CK gets its value either from CE or from CIE. And the case depends on the kind of CK.

Perhaps it would be more expressive (?) : assert(!CIE && CE && "Inherited constructors cannot be complete!");
Alternatively maybe the assert above in line 400 could be assert(!CE != !CIE); to express that it is either inherited or not but cannot be both (logical xor).

NoQ marked 2 inline comments as done.Feb 18 2020, 8:21 AM

If the AST is hard to work with would it make sense to try to change the AST a bit?

Hmm, you mean making CXXConstructExpr an abstract base with subclasses CXXSimpleConstructExpr and CXXInheritedConstructExpr? For me it seems reasonable but I am afraid it would be a huge impact to clang, and not only to the analyzer.

I'm not extremely confident of my ability to write down the argument expressions correctly (which is the bigger problem here), especially when arguments need to be constructed. Like, not sure if it's harder to get right than this patch, but it's definitely easier to get wrong.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
436

The big question here is, should assert messages explain what happens when they fail, or why shouldn't they fail? I.e., assert(CE && !CIE && "A complete constructor is inherited!"); or assert(CE && !CIE && "A complete constructor cannot be inherited!");? I think it's the former because that produces a more understandable crash dump. In this sense, i agree that the assert is confusing.

Alternatively maybe the assert above in line 400 could be assert(!CE != !CIE);

Dunno, that's kinda obvious because they're dyn_casts to unrelated classes (and thankfully we don't have much multiple inheritance in the AST).

clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
31

The focus is on how B::B(S, int) has an argument of type S that needs to be constructed. So it's kinda constructors_with_arguments_with_constructors.

martong added inline comments.Feb 18 2020, 8:39 AM
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
436

Ok. assert(CE && !CIE && "A complete constructor is inherited!"); sounds good to me :)

NoQ marked 6 inline comments as done.Feb 23 2020, 2:08 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
891

Dunno, it's kinda obvious that it has some constructor from an integer, and that's really the only thing that we need to know about it. Also what's the proper way to make a line break in \c? Because i always forget how to build with doxygen :)

915

Maybe but sounds complicated. There isn't *that* much duplication. Most of the code is either inherited from a superclass or is actually different.

I guess a common superclass for CXXConstructorCall and CXXInheritedConstructorCall should be sufficient for most purposes.

Yeah, let me actually do this.

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
545

Ah yes, I'd love me some good ol' RelaxedAliveVariablesAnalysis (:

Dunno why the terminology is made that way (i.e., live as in "live music"), but it seems to be that way :/

NoQ updated this revision to Diff 246127.Feb 23 2020, 2:48 PM
NoQ marked an inline comment as done.
NoQ added a reviewer: martong.
NoQ removed a subscriber: martong.
  • Update AnyCall to support inherited constructors as well. This fixes a crash in RetainCountChecker.
  • Introduce a common abstract base class for constructor call events.
  • Address comments^^
Charusso accepted this revision.Feb 23 2020, 3:21 PM

I think on a long term our knowledge increases so we could generalize upon what we have learnt. For now as long as it is working and have tests, it is cool.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
891

The simplest way is the Markdown support with triple ` above and below the code-snippet: http://www.doxygen.nl/manual/markdown.html
The other more commonly used way is \code: http://www.doxygen.nl/manual/commands.html#cmdcode

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
497

This assertion has been removed intentionally?

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
545

I like the wording live.

This revision is now accepted and ready to land.Feb 23 2020, 3:21 PM
NoQ marked an inline comment as done.Feb 23 2020, 4:26 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
497

Yup. I think this isn't a valuable assertion because (1) the invariant it documents ("inherited constructors don't require additional tracking in the program state so far") is accidental rather than intentional (we may add other kinds of tracking later) and (2) the code behaves reasonably well even if the assertion fails (given that i replaced CE with E in the generateNode invocation).

This revision was automatically updated to reflect the committed changes.
martong added inline comments.Mar 2 2020, 9:51 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
925

This line causes a regression in our CTU jobs, because it seems getCallSite can return with a null Stmt*. And then cast fires an assertion. I am going to do a simple fix by changing cast to cast_or_null if there is no objection.

martong added inline comments.Mar 3 2020, 2:54 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
925

Hmm, it is not that simple unfortunately. The use of cast_or_null would result in a null dereference in getNumArgs(). I started to investigate how is it possible that getCallSite() returns with a nullptr.

@NoQ

I've found the following reproducer to crash in CallAndMessageChecker:

class a {
public:
  a(int);
};
struct b : a {
  using a::a;
};
void c() {
  int d;
  b x(d); //Crash!, Note, x(0) causes no crash
}

I am working on a fix, but any insight and help from you is really appreciated.

NoQ added a comment.Mar 3 2020, 4:59 PM

@NoQ

I've found the following reproducer to crash in CallAndMessageChecker:

class a {
public:
  a(int);
};
struct b : a {
  using a::a;
};
void c() {
  int d;
  b x(d); //Crash!, Note, x(0) causes no crash
}

I am working on a fix, but any insight and help from you is really appreciated.

Uh-oh, i've been looking for those but never found them.

If you do -analyzer-display-progress you'll see that it crashes not in c() but in b::a(), i.e. when trying to analyze an inheriting constructor as a top-level function. It then crashes when it's trying to figure out which argument expressions do we pass into the inherited constructor, but is unable to do that because the answer is "the same arguments we've received" but we don't know which arguments we've received, because, well, we are the top-level call, so no expressions for us.

I believe we simply should not try to analyze inheriting constructors as top-level functions. We won't be able to even display any diagnostics, given that the inheriting constructor doesn't have a body.

I thereby leave out the reason why x(d) and x(0) demonstrate different behavior as a simple exercise to the reader :]

rsmith added a subscriber: rsmith.Mar 4 2020, 5:51 PM

There are also no argument expressions, even though the inherited constructor for A takes one argument x. The current patch takes argument expressions from the topmost CXXConstructExpr, which also requires a different location context to access. As far as i can see, the result of such substitution is always correct, i.e., the resulting argument SVal obtained from the Environment this way will always be evalBinOp-equal to the correct argument value.

Just in case this isn't clear: the reason why CXXInheritedCtorInitExpr doesn't take arguments and doesn't model parameter initialization is because there is none: the arguments in the outer CXXConstructExpr directly initialize the parameters of the base class constructor, and no copies are made. (Making a copy of the parameter is incorrect, at least if it's done in an observable way.) The derived class constructor doesn't even exist in the formal model.

Eg, in:

struct X { X *p = this; ~X() {} };
struct A { A(X x) : b(x.p == &x) {} bool b; };
struct B : A { using A::A; };
B b = X{};

... b.b is initialized to true.

NoQ added a comment.Mar 4 2020, 8:15 PM

Just in case this isn't clear: the reason why CXXInheritedCtorInitExpr doesn't take arguments and doesn't model parameter initialization is because there is none: the arguments in the outer CXXConstructExpr directly initialize the parameters of the base class constructor, and no copies are made. (Making a copy of the parameter is incorrect, at least if it's done in an observable way.) The derived class constructor doesn't even exist in the formal model.

Damn, that's interesting! I wonder if the rest of our code will find it less surprising if we omit this whole stack frame entirely.

Thanks Richard for the explanation.

Artem, I think this justifies your suggestion to skip the analysis of inherited constructors as top level functions. I just created the patch https://reviews.llvm.org/D75678

This patch introduced a crash while I was analyzing the libpressio.
I was using the CodeChecker to drive the analysis with the --enable-all flag.

The exact command was the following:

/home/username/git/llvm-project/build/debug/bin/clang-11 --analyze -Qunused-arguments -Xclang -analyzer-opt-analyze-headers -Xclang -analyzer-output=plist-multi-file -o /home/username/git/libpressio/build/results/pressio_options.cc_clangsa_0316a939d2e5f7ba700a67a7cc467d92.plist -Xclang -analyzer-config -Xclang expand-macros=true -Xclang -analyzer-checker=apiModeling.StdCLibraryFunctions -Xclang -analyzer-checker=apiModeling.TrustNonnull -Xclang -analyzer-checker=apiModeling.google.GTest -Xclang -analyzer-checker=apiModeling.llvm.CastValue -Xclang -analyzer-checker=apiModeling.llvm.ReturnValue -Xclang -analyzer-checker=core.CallAndMessage -Xclang -analyzer-checker=core.DivideZero -Xclang -analyzer-checker=core.DynamicTypePropagation -Xclang -analyzer-checker=core.NonNullParamChecker -Xclang -analyzer-checker=core.NonnilStringConstants -Xclang -analyzer-checker=core.NullDereference -Xclang -analyzer-checker=core.StackAddrEscapeBase -Xclang -analyzer-checker=core.StackAddressEscape -Xclang -analyzer-checker=core.UndefinedBinaryOperatorResult -Xclang -analyzer-checker=core.VLASize -Xclang -analyzer-checker=core.builtin.BuiltinFunctions -Xclang -analyzer-checker=core.builtin.NoReturnFunctions -Xclang -analyzer-checker=core.uninitialized.ArraySubscript -Xclang -analyzer-checker=core.uninitialized.Assign -Xclang -analyzer-checker=core.uninitialized.Branch -Xclang -analyzer-checker=core.uninitialized.CapturedBlockVariable -Xclang -analyzer-checker=core.uninitialized.UndefReturn -Xclang -analyzer-checker=cplusplus.InnerPointer -Xclang -analyzer-checker=cplusplus.Move -Xclang -analyzer-checker=cplusplus.NewDelete -Xclang -analyzer-checker=cplusplus.NewDeleteLeaks -Xclang -analyzer-checker=cplusplus.PlacementNew -Xclang -analyzer-checker=cplusplus.PureVirtualCall -Xclang -analyzer-checker=cplusplus.SelfAssignment -Xclang -analyzer-checker=cplusplus.SmartPtr -Xclang -analyzer-checker=cplusplus.VirtualCallModeling -Xclang -analyzer-checker=deadcode.DeadStores -Xclang -analyzer-checker=fuchsia.HandleChecker -Xclang -analyzer-checker=nullability.NullPassedToNonnull -Xclang -analyzer-checker=nullability.NullReturnedFromNonnull -Xclang -analyzer-checker=nullability.NullabilityBase -Xclang -analyzer-checker=nullability.NullableDereferenced -Xclang -analyzer-checker=nullability.NullablePassedToNonnull -Xclang -analyzer-checker=nullability.NullableReturnedFromNonnull -Xclang -analyzer-checker=optin.cplusplus.UninitializedObject -Xclang -analyzer-checker=optin.cplusplus.VirtualCall -Xclang -analyzer-checker=optin.mpi.MPI-Checker -Xclang -analyzer-checker=optin.osx.OSObjectCStyleCast -Xclang -analyzer-checker=optin.osx.cocoa.localizability.EmptyLocalizationContextChecker -Xclang -analyzer-checker=optin.osx.cocoa.localizability.NonLocalizedStringChecker -Xclang -analyzer-checker=optin.performance.GCDAntipattern -Xclang -analyzer-checker=optin.performance.Padding -Xclang -analyzer-checker=optin.portability.UnixAPI -Xclang -analyzer-checker=security.FloatLoopCounter -Xclang -analyzer-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling -Xclang -analyzer-checker=security.insecureAPI.SecuritySyntaxChecker -Xclang -analyzer-checker=security.insecureAPI.UncheckedReturn -Xclang -analyzer-checker=security.insecureAPI.bcmp -Xclang -analyzer-checker=security.insecureAPI.bcopy -Xclang -analyzer-checker=security.insecureAPI.bzero -Xclang -analyzer-checker=security.insecureAPI.decodeValueOfObjCType -Xclang -analyzer-checker=security.insecureAPI.getpw -Xclang -analyzer-checker=security.insecureAPI.gets -Xclang -analyzer-checker=security.insecureAPI.mkstemp -Xclang -analyzer-checker=security.insecureAPI.mktemp -Xclang -analyzer-checker=security.insecureAPI.rand -Xclang -analyzer-checker=security.insecureAPI.strcpy -Xclang -analyzer-checker=security.insecureAPI.vfork -Xclang -analyzer-checker=unix.API -Xclang -analyzer-checker=unix.DynamicMemoryModeling -Xclang -analyzer-checker=unix.Malloc -Xclang -analyzer-checker=unix.MallocSizeof -Xclang -analyzer-checker=unix.MismatchedDeallocator -Xclang -analyzer-checker=unix.Vfork -Xclang -analyzer-checker=unix.cstring.BadSizeArg -Xclang -analyzer-checker=unix.cstring.CStringModeling -Xclang -analyzer-checker=unix.cstring.NullArg -Xclang -analyzer-checker=valist.CopyToSelf -Xclang -analyzer-checker=valist.Uninitialized -Xclang -analyzer-checker=valist.Unterminated -Xclang -analyzer-checker=valist.ValistBase -Xclang -analyzer-config -Xclang aggressive-binary-operation-simplification=true -Xclang -analyzer-config -Xclang crosscheck-with-z3=true -x c++ --target=x86_64-linux-gnu -std=gnu++14 -Dlibpressio_EXPORTS -I/home/username/git/libpressio/include -I/home/username/git/libpressio/build/include -O3 -fPIC -std=gnu++17 -isystem /usr/include/c++/9 -isystem /usr/include/x86_64-linux-gnu/c++/9 -isystem /usr/include/c++/9/backward -isystem /usr/local/include -isystem /usr/include/x86_64-linux-gnu -isystem /usr/include /home/username/git/libpressio/src/pressio_options.cc

The top 25 frame of the call stack in GDB was:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fffeeb9e801 in __GI_abort () at abort.c:79
#2  0x00007fffeeb8e39a in __assert_fail_base (fmt=0x7fffeed157d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7fffe4662d48 "Val && \"isa<> used on a null pointer\"", 
    file=file@entry=0x7fffe465d910 "../../llvm/include/llvm/Support/Casting.h", line=line@entry=104, 
    function=function@entry=0x7fffe4663d90 "static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = clang::CXXInheritedCtorInitExpr; From = clang::Stmt]") at assert.c:92
#3  0x00007fffeeb8e412 in __GI___assert_fail (assertion=0x7fffe4662d48 "Val && \"isa<> used on a null pointer\"", file=0x7fffe465d910 "../../llvm/include/llvm/Support/Casting.h", line=104, 
    function=0x7fffe4663d90 "static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = clang::CXXInheritedCtorInitExpr; From = clang::Stmt]") at assert.c:101
#4  0x00007fffe493cc5b in llvm::isa_impl_cl<clang::CXXInheritedCtorInitExpr, clang::Stmt const*>::doit (Val=0x0) at ../../llvm/include/llvm/Support/Casting.h:104
#5  0x00007fffe493b450 in llvm::isa_impl_wrap<clang::CXXInheritedCtorInitExpr, clang::Stmt const*, clang::Stmt const*>::doit (Val=@0x7fffffff7bd0: 0x0) at ../../llvm/include/llvm/Support/Casting.h:131
#6  0x00007fffe4938d89 in llvm::isa_impl_wrap<clang::CXXInheritedCtorInitExpr, clang::Stmt const* const, clang::Stmt const*>::doit (Val=@0x7fffffff7c28: 0x0)
    at ../../llvm/include/llvm/Support/Casting.h:122
#7  0x00007fffe4935e6a in llvm::isa<clang::CXXInheritedCtorInitExpr, clang::Stmt const*> (Val=@0x7fffffff7c28: 0x0) at ../../llvm/include/llvm/Support/Casting.h:142
#8  0x00007fffe492c2f1 in clang::ento::CXXInheritedConstructorCall::getInheritingStackFrame (this=0x55555755dbe8) at ../../clang/lib/StaticAnalyzer/Core/CallEvent.cpp:924
#9  0x00007fffe4932d70 in clang::ento::CXXInheritedConstructorCall::getInheritingConstructor (this=0x55555755dbe8) at ../../clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:932
#10 0x00007fffe4932d9a in clang::ento::CXXInheritedConstructorCall::getNumArgs (this=0x55555755dbe8) at ../../clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:936
#11 0x00007fffe52e06b8 in (anonymous namespace)::CallAndMessageChecker::checkPreCall (this=0x555555684e50, Call=..., C=...) at ../../clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp:404
#12 0x00007fffe52e21c8 in clang::ento::check::PreCall::_checkCall<(anonymous namespace)::CallAndMessageChecker> (checker=0x555555684e50, msg=..., C=...)
    at ../../clang/include/clang/StaticAnalyzer/Core/Checker.h:168
#13 0x00007fffe494f0da in clang::ento::CheckerFn<void (clang::ento::CallEvent const&, clang::ento::CheckerContext&)>::operator()(clang::ento::CallEvent const&, clang::ento::CheckerContext&) const (
    this=0x7fffffff8020, ps#0=..., ps#1=...) at ../../clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:69
#14 0x00007fffe4946b30 in (anonymous namespace)::CheckCallContext::runChecker (this=0x7fffffff8260, checkFn=..., Bldr=..., Pred=0x55555755db60)
    at ../../clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:291
#15 0x00007fffe494a0f9 in expandGraphWithCheckers<(anonymous namespace)::CheckCallContext> (checkCtx=..., Dst=..., Src=...) at ../../clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:139
#16 0x00007fffe4946c07 in clang::ento::CheckerManager::runCheckersForCallEvent (this=0x555555673eb0, isPreVisit=true, Dst=..., Src=..., Call=..., Eng=..., WasInlined=false)
    at ../../clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:308
#17 0x00007fffe49ccbcf in clang::ento::CheckerManager::runCheckersForPreCall (this=0x555555673eb0, Dst=..., Src=..., Call=..., Eng=...)
    at ../../clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:274
#18 0x00007fffe49c9a72 in clang::ento::ExprEngine::handleConstructor (this=0x7fffffff9240, E=0x7fffe187cbb8, Pred=0x55555755db60, destNodes=...)
    at ../../clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:551
#19 0x00007fffe49ca076 in clang::ento::ExprEngine::VisitCXXInheritedCtorInitExpr (this=0x7fffffff9240, CE=0x7fffe187cbb8, Pred=0x55555755db60, Dst=...)
    at ../../clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:627
#20 0x00007fffe4997c85 in clang::ento::ExprEngine::Visit (this=0x7fffffff9240, S=0x7fffe187cbb8, Pred=0x55555755db60, DstTop=...) at ../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1623
#21 0x00007fffe499385c in clang::ento::ExprEngine::ProcessStmt (this=0x7fffffff9240, currStmt=0x7fffe187cbb8, Pred=0x55555755da88) at ../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:791
#22 0x00007fffe4992af2 in clang::ento::ExprEngine::processCFGElement (this=0x7fffffff9240, E=..., Pred=0x55555755da88, StmtIdx=0, Ctx=0x7fffffff8ea0)
    at ../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:637
#23 0x00007fffe496900e in clang::ento::CoreEngine::HandleBlockEntrance (this=0x7fffffff9260, L=..., Pred=0x55555755da88) at ../../clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:290
#24 0x00007fffe4968523 in clang::ento::CoreEngine::dispatchWorkItem (this=0x7fffffff9260, Pred=0x55555755da88, Loc=..., WU=...) at ../../clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:163
#25 0x00007fffe49683ed in clang::ento::CoreEngine::ExecuteWorkList (this=0x7fffffff9260, L=0x5555579d0320, Steps=224998, InitState=...) at ../../clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:148

Which is might caused by a dyn_cast.

I'm using the clang of 95a94df5a9c3d7d2aa92b6beb13e82d8d5832e2e commit hash.
My GCC version is gcc (Ubuntu 9.2.1-17ubuntu1~18.04.1) 9.2.1 20191102

Breaking on the SIGABRT signal in GDB and examining the source location of the place:

(gdb) p Call.getSourceRange().dump(C.getSourceManager())
</usr/include/c++/9/variant:580:20>

Where the code was something like this (the full source code available on github gcc repo):

template<bool, typename... _Types>
struct _Copy_assign_base : _Move_ctor_alias<_Types...>
{
  using _Base = _Move_ctor_alias<_Types...>;
  using _Base::_Base;
//^^^^^^^^^^^^^^^^^^

  _Copy_assign_base&
  operator=(const _Copy_assign_base& __rhs) noexcept(_Traits<_Types...>::_S_nothrow_copy_assign)
  {
[...]

Sorry if this is not the right place for the report. @NoQ

@steakhal

There is a fix for the crash: https://reviews.llvm.org/D75678
Let's hope that patch get's in soon.

steakhal added inline comments.Nov 3 2020, 4:16 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
909–911

Why is this function virtual?
If we want such behavior we should mark the CallEvent::getOriginExpr virtual and just override it here.
As-of-now, this just hides the previous implementation, causing potential problems.

This code-smell occures several times across this class hierachy.

Is this the expected behavior @NoQ?

NoQ added inline comments.Nov 3 2020, 1:30 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
909–911

I think you're right, it should have been virtual from the start. Weird. Probably the only reason this works is that all overrides do literally the same thing, just cast to different types. But if even one of them doesn't we're in trouble. Yeah this needs to be fixed.

steakhal added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
909–911

D90754 aims to fix this. Thank you.