Page MenuHomePhabricator

[analyzer] NFC: Avoid element regions of void type.
ClosedPublic

Authored by NoQ on Dec 6 2017, 6:49 PM.

Details

Summary

Add an assertion that ElementRegion's element type is not void.

Fix two violations of this rule that shown up on four of our existing test files.

I accidentally noticed that problem when i was looking at how c++-allocator-inlining mode handles int *x = new int(5) - apparently, it binds 5 to such broken region if a void pointer is returned by operator new(), which is a regression compared to the default mode. But this patch doesn't fix it (there were no tests for that).

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Dec 6 2017, 6:49 PM
NoQ added a comment.Dec 6 2017, 7:09 PM

I accidentally noticed that problem

The actual problem was that RegionStore was unable to retrieve the binding to such ElementRegion (in case of this example, 5 S32b). There may be more problems with such regions. But at the same time i believe that the very idea of a void ElementRegion is frightening enough to be fixed on its own.

xazax.hun edited edge metadata.Dec 7 2017, 7:50 AM

I like the idea of adding those assertions but a bit worried about the other changes. Basically (if I get this right), we are masking the issues here and I am a bit afraid that they will get forgotten. I think it would be nice to at least add a FIXME that this path should never be triggered.

NoQ added a comment.Dec 7 2017, 8:15 AM

I think the new behavior is correct in the sense that in our region hierarchy byte offsets (such as arithmetic on void pointers) are normally represented as char-type element regions. For instance, we have a similar mechanism is implemented in pointer casts case, when the byte offset of the pointer is not divisible by the casted object size: we just add a character element region to account for the remainder of the offset.

Like, it's not the situation when we couldn't figure out the type - it would have been null in that case. Here we know exactly that the type is void. And when you add N to a void pointer, it changes by N bytes, which is what we should, in my opinion, try to represent somehow. So i believe that the newly constructed SVals are correctly representing their respective expressions or arithmetic results, and i'm not immediately seeing a better behavior.

In D40939#948252, @NoQ wrote:

Like, it's not the situation when we couldn't figure out the type - it would have been null in that case. Here we know exactly that the type is void.

Oh, thank you for the clarification!

NoQ updated this revision to Diff 125972.Dec 7 2017, 8:49 AM

Rebase on top of D40584.

Stronger assertion for CXXThisRegion type. Uhm, forgot to mention that i also added a similar assertion to CXXThisRegion (which didn't find any bugs yet). Other typed value regions don't store types directly.

dcoughlin accepted this revision.Dec 18 2017, 10:45 AM

Looks good to me, although I have a super nit about wording in a comment.

lib/StaticAnalyzer/Core/ExprEngine.cpp
2215

Super nit: Since we can't actually have lvalues for a forbidden lvalue type, we should instead call it a 'location' in the comment. "We still need to have sensible locations to represent this stuff".

This revision is now accepted and ready to land.Dec 18 2017, 10:45 AM
NoQ updated this revision to Diff 127548.Dec 19 2017, 9:26 AM
  • Fix comments as suggested by Devin.
  • Point out that arithmetic on void pointers is a GNU extension.

No tests?

This patch adds an assertion => Multiple existing tests immediately starts crashing => This patch fixes the crashes with the new functionality.

So in my understanding the new assertion is all the tests we need. Old tests didn't pass when this assertion was enforced, now they do. If somebody breaks the newly added functionality, the regression will immediately show up on tests by hitting the assertion.

Additionally, i'm adding actual practical tests (of how the analyzer's behavior actually change) in a follow-up patch (D41250). So if we merge these two patches (if necessary), we'd have new stuff in the tests directory.

Or i could try to come up with actual tests for this patch, but i'm not sure it's worth it given the above.

Please correct me if my logic is flawed or contradicts existing policies.

NoQ retitled this revision from [analyzer] Avoid element regions of void type. to [analyzer] NFC: Avoid element regions of void type..Dec 19 2017, 2:43 PM

I guess i'd commit it together with D41250 in a single commit, so that there obviously were tests.

NoQ updated this revision to Diff 129211.Jan 9 2018, 7:25 PM

Rebase.

This revision was automatically updated to reflect the committed changes.