Page MenuHomePhabricator

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

Authored by NoQ on Fri, Jul 26, 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

Event Timeline

NoQ created this revision.Fri, Jul 26, 6:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 26, 6:20 PM
NoQ marked an inline comment as done.Fri, Jul 26, 6:23 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
157

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.Fri, Jul 26, 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
406

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

641

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

1707–1708

Please, update the comment as well.

1798–1799

This one as well.

2021

Please add a comment to the block as well.

clang/test/Analysis/main.c
22

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
641

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

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

Fxd!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
641

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
157

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

225

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.

404

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.

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

Fxd.