[analyzer] Ignore IncompleteArrayTypes in getStaticSize() for FAMs

[steakhal]

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.

[martong]

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]

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]

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]

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]

• 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]

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]

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]

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.

[steakhal]

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