This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Ignore IncompleteArrayTypes in getStaticSize() for FAMs
ClosedPublic

Authored by steakhal on Jun 30 2021, 4:30 AM.

Details

Summary

Currently only ConstantArrayType is considered for flexible array
members (FAMs) in getStaticSize().
However, IncompleteArrayType also shows up in practice as FAMs.

This patch will ignore the IncompleteArrayType and return Unknown
for that case as well. This way it will be at least consistent with
the current behavior until we start modeling them accurately.

I'm expecting that this will resolve a bunch of false-positives internally,
caused by the ArrayBoundV2.

Diff Detail

Event Timeline

steakhal created this revision.Jun 30 2021, 4:30 AM
steakhal requested review of this revision.Jun 30 2021, 4:30 AM

Thanks!

clang/test/Analysis/out-of-bounds-new.cpp
171 ↗(On Diff #355504)

Should we have test cases with data[1] (and data[0] if that makes sense at all) ?
Would data[1] behave the same way? And if not, even then perhaps we should have test cases just to document that.

steakhal added inline comments.Jun 30 2021, 5:24 AM
clang/test/Analysis/out-of-bounds-new.cpp
171 ↗(On Diff #355504)

We could add tests for them in a subsequent patch.
But strictly speaking, those arrays would be of ConstantArrayType types.
That being said, those were indistinguishable from FAMs or just a regular array of size N.

Maybe we can figure out some heuristics driving this, but that's definitely out of the scope of this patch.

martong added inline comments.Jul 1 2021, 9:53 AM
clang/test/Analysis/out-of-bounds-new.cpp
169 ↗(On Diff #355504)

We could add tests for them in a subsequent patch.

Strictly speaking, flexible array members are defined in C99 (6.7.2.1), thus I think we should have these tests at least with a different RUN line (i.e. with -std=c99) or in a different file. But if we create a new test file then that could easily handle data[1] and data[0] cases as well.
So I am okay with this change if I can see the subsequent patch in the stack.

ASDenysPetrov added a comment.EditedJul 19 2021, 4:00 AM

The fix seems pretty resonable to me. Indeed IncompleteArrayType can happen according to this paper https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
I'd like to see more test cases mentioned by @martong.

steakhal updated this revision to Diff 366964.Aug 17 2021, 11:33 AM
  • wrapped the FAM detection into a lambda, which could be extended later if needed
  • added test cases for: [0], [] and [1] array members, on the stack (automatic storage), alloca malloc using a bunch of C and C++ versions.
ASDenysPetrov added inline comments.Aug 18 2021, 6:56 AM
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
776–779

I think we can omit getAsArrayType, since we check for certain array types anyway.

clang/test/Analysis/flexible-array-members.c
7–11

AFAIK, FAM is not a part of C++ Standard. Does clang support it as extension?

74

I'd like to use another naming here, because int data[1] is not actually a flexible array.

steakhal marked 2 inline comments as done.Aug 18 2021, 7:37 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
776–779

No, we can't. ASTContext::getAsArrayType() returns a canonical type, which could be used later by the regular isa/dyn_cast API, but you cannot use them on Ty.
In fact, first I tried what you proposed, but I hit a static assertion checking for exactly this.

clang/test/Analysis/flexible-array-members.c
7–11

You are completely correct.

74

What do you think would be a better fit?
I already highlighted this at the variable declaration with the name likely_fam - at least that was my concept.

ASDenysPetrov added inline comments.Aug 18 2021, 8:43 AM
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
776–779

I see. OK, then.

clang/test/Analysis/flexible-array-members.c
74

You can name it as SAM kinda Static Array Member or AM - Array Member. Whatever. My point is just to be closer to the meaning. But you can ignore this if you want, because we can understand what you mean from the context.

ASDenysPetrov accepted this revision.Aug 18 2021, 11:10 AM
This revision is now accepted and ready to land.Aug 18 2021, 11:10 AM
steakhal marked 2 inline comments as done.Aug 25 2021, 4:20 AM

Thank you for the review @ASDenysPetrov! I'd like to commit this in its current form.

Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 7:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript