Page MenuHomePhabricator

[analyzer] C++17: PR40022: Support aggregate initialization with compound values in presence of base classes.
ClosedPublic

Authored by NoQ on Mar 6 2019, 2:36 PM.

Details

Summary

I'll add more tests soon, to see if it is actually initialized *correctly* now, but for now here's the code that fixes the crash in https://bugs.llvm.org/show_bug.cgi?id=40022 .

It turns out that C++17 aggregate initialization with base classes wasn't really ever implemented for the case when it *doesn't* involve calling any sub-object constructors. That it, when the aggregate is initialized with a simple initializer list and the resulting value is a nonloc::CompoundVal, we simply didn't know that we need to perform the store bind for the base classes as well. Now it should work.

The original bug report causes us to crash when there's a flexible array at the end of the structure (which is a non-standard thing), which made me underestimate the problem a bit :)

The case where we *do* have constructors in base classes is, of course, still unimplemented; it was last touched in D40841.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Mar 6 2019, 2:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 2:36 PM
alexfh added a subscriber: alexfh.Mar 7 2019, 5:37 AM

LG in general. A couple of comments.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
2348 ↗(On Diff #189594)

Should this be const auto&?

2349 ↗(On Diff #189594)

!B.isVirtual()

Consider also adding && "more details about this assertion".

At first glance this looks OK, but I'll take the time to understand the infrastructure behind it and give a proper review.

clang/test/Analysis/array-struct-region.cpp
3 ↗(On Diff #189594)

I didn't see -analyzer-config c++-inlining=constructors in the bugreport -- why is it needed?

NoQ updated this revision to Diff 189781.Mar 7 2019, 2:15 PM
NoQ marked 5 inline comments as done.

Thx! Also added the more direct test.

I'll take the time to understand the infrastructure behind it

Well, roughly, the CompoundVal kind of value is used very rarely, unlike LazyCompoundVal. The raw CompoundVal is essentially a symbolic InitListExpr: an (immutable) list of other values. It appears pretty much only when there's an actual initializer list expression in the program, and the analyzer tries to unwrap it as soon as possible.

This code is where such unwrap happens: when the compound value is put into the object that it was supposed to initialize (it's an *initializer* list, after all), instead of binding the whole value to the whole object, we bind sub-values to sub-objects. Sub-values may themselves be compound values, and in this case the procedure becomes recursive.

The annoying part about compound values is that they don't carry any sort of information about which value corresponds to which sub-object. It's simply a list of values in the middle of nowhere; the Store expects to match them to sub-objects, essentially, by *index*: first value binds to the first field, second value binds to the second field, etc. The crash was occuring when it was accidentally trying to bind a base-class value to a field object - because it simply didn't know that base objects are a thing.

NoQ added inline comments.Mar 7 2019, 2:25 PM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
2348 ↗(On Diff #189594)

Whoops. Also i still somehow hate it that these aren't pointers :)

clang/test/Analysis/array-struct-region.cpp
3 ↗(On Diff #189594)

Hmm, right, these flags are outdated long time ago. Wiped them out from this test.

Ooooh you explained that very well, I'm clear on what's happening then :D Still, I'll poke the code here and there to learn about this part of the analyzer, I might even come up with some meaningful feedback (or a meaningful patch acceptance).

NoQ updated this revision to Diff 189791.Mar 7 2019, 3:28 PM

Added this as a comment.

If the patch is not obvious at a glance, feel free to demand comments! Every second you spend trying to figure out how the code works is multiplied by the number of people who will have to do the same in the future.

This revision is now accepted and ready to land.Mar 7 2019, 3:34 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 5:21 PM