This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Trust global initializers when analyzing main().
ClosedPublic

Authored by NoQ on Jul 26 2019, 6:20 PM.

Details

Summary

This is inspired by https://bugs.llvm.org/show_bug.cgi?id=42780, even though it doesn't fix the bug.

If the global variable has an initializer, we'll ignore it because we're normally not analyzing the program from the beginning. This means that a global variable may have changed before we start our analysis.

However when we're analyzing main() as the top-level function, we can rely on global initializers to still be valid. At least in C; in C++ we have global constructors for breaking this logic. In C we can also have global constructors as a GNU extension, but i'd rather not worry about that.

In this patch i don't try to evaluate global initializers; the patch only covers the case when they're constants. This is the reason why we don't cover https://bugs.llvm.org/show_bug.cgi?id=42780, however we could cover it if SValBuilder.getConstantVal() was a bit more feature-rich (namely, we need to be able to constant-evaluate expressions like &var.field where var is a global variable; it's a concrete region (i.e., without symbolic base) value in our model, so we can make getConstantVal() powerful enough to emit it).

For initializers of fields and elements i'm piggybacking on @r.stahl's work to cover global constant initializers (D45774).

The main benefit of this patch is to make it harder for people to send us false positive reproducers that don't actually reproduce the false positive that they're having :)

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jul 26 2019, 6:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 6:20 PM
NoQ marked an inline comment as done.Jul 26 2019, 6:23 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
157 ↗(On Diff #212030)

We could have put this flag into RegionStoreManager instead - after all, it doesn't change for the whole duration of the analysis. It would have made it easier to handle (avoid weird bitwise logic) and probably cheaper, but it'll make the manager needlessly stateful (which wouldn't really hurt at all right now, just looks ugly).

NoQ updated this revision to Diff 212035.Jul 26 2019, 6:25 PM

Actually add the logic for C++.

Thank you for working on this! I agree, we should trust global initializers in main() in C. Can we maybe detect whether GNU global constructors are enabled or better: used?

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
395 ↗(On Diff #212035)

I think this function deserves now some doc comments since it is not a trivial getter anymore.

630 ↗(On Diff #212035)

(uintptr_t)1 look like a bit like some kind of magic number. Could we define it as a constant instead?

1693 ↗(On Diff #212035)

Please, update the comment as well.

1785 ↗(On Diff #212035)

This one as well.

2005 ↗(On Diff #212035)

Please add a comment to the block as well.

clang/test/Analysis/main.c
21 ↗(On Diff #212035)

Please add a negative test case (i.e. function that is not main) as well.

I like the change.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
630 ↗(On Diff #212035)

Or maybe this can be abstracted away using llvm::PointerIntPair?

NoQ updated this revision to Diff 214470.Aug 9 2019, 3:48 PM
NoQ marked 8 inline comments as done.

Fxd!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
630 ↗(On Diff #212035)

Or maybe this can be abstracted away using llvm::PointerIntPair?

!!!

I really like the high level idea proposed by this patch, and the test files make me believe that its correct as well! I'm really not familiar around this part of the code, so if its okay, I'll take my time to do the usual find references, inserting unreachables/asserts to gain a better understanding before the green checkmark.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
215 ↗(On Diff #214470)

To me personally, reinterpret_cast is a louder shout about having to use statically unverifiable casts that may be dangerous. I don't insist on it however.

394 ↗(On Diff #214470)

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Don’t duplicate function or class name at the beginning of the comment.

Though I'll concede to the argument that we're keeping consistency.

157 ↗(On Diff #212030)

Could you please add some comments to this? We'll forget about this name I fear in no time.

NoQ updated this revision to Diff 215274.Aug 14 2019, 3:42 PM
NoQ marked 3 inline comments as done.

Fxd.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 28 2019, 11:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2019, 11:44 AM

Sorry for not accepting this -- I actually read the code multiple times and didn't see anything that stood out! I didn't have the time to dig too deep, but if the tests are anything to go by, its gotta be alright.

NoQ added a comment.Aug 28 2019, 12:04 PM

Np, post-commit review is always welcome (: