Page MenuHomePhabricator

[analyzer] Retrieve incomplete array extent from its redeclaration.
ClosedPublic

Authored by ASDenysPetrov on Oct 11 2021, 6:28 AM.

Details

Summary

Fix a case when the extent can not be retrieved correctly from incomplete array declaration. Use redeclaration to get the array extent.

Diff Detail

Event Timeline

ASDenysPetrov created this revision.Oct 11 2021, 6:28 AM
ASDenysPetrov requested review of this revision.Oct 11 2021, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2021, 6:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ASDenysPetrov added a comment.EditedOct 11 2021, 6:34 AM

@NoQ @martong
Guys, I've patched the bug (mentioned in D104285). Could you check it? It works, but I have doubts using VR->getDecl()->getMostRecentDecl(); as a correct way to get a right declaration.

steakhal added inline comments.Oct 11 2021, 7:39 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

I think you supposed to use the getCanonicalDecl() instead.

1764

What does the function return if this branch is not taken?
Shouldn't you handle IncompleteArrayType as well?

1766

I would favor not adding another level of indentation xD

clang/test/Analysis/initialization.c
101

Why is this a VLA? I thought it's just an IncompleteArrayType.
Could you please make sure that this is actually a VLA?

@steakhal
Thank you for your comments.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Using getCanonicalDecl does not fix the issue. I've checked.

1764

What does the function return if this branch is not taken?

In case of a false branch it goes down the code to getBindingForFieldOrElementCommon.

Shouldn't you handle IncompleteArrayType as well?

Only ConstantArrayType has getSize() method. And getMostRecentDecl give us an ConstantArrayType even if it is declared as IncompleteArrayType, but getCanonicalDecl does not.

1766

It annoys me as well. I could use goto but I suppose you won't like it even more. :-)

clang/test/Analysis/initialization.c
101

I mixed up in terminilogy. I'll fix it.

ASDenysPetrov retitled this revision from [analyzer] Retrieve VLA extent from its redeclaration. to [analyzer] Retrieve incomplete array extent from its redeclaration..
ASDenysPetrov edited the summary of this revision. (Show Details)
balazske added inline comments.
clang/test/Analysis/initialization.c
102

It is possible to add a line const int glob_arr3[]; again and then it may not work?

NoQ added inline comments.Oct 11 2021, 7:56 PM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Aha ok, can you try iterating over redecls()?

Separately, I suspect that this should be performed before the VarRegion is constructed in the first place. Maybe in its constructor we should assert(VD->isThisDeclarationADefinition()) or something like that.

ASDenysPetrov added inline comments.Oct 12 2021, 4:39 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Aha ok, can you try iterating over redecls()?

Do you assume that in a list {redecl1, redecl2, redecl3} redecl2 may be our guy but 1 and 3 may not?

Separately, I suspect that this should be performed before the VarRegion is constructed in the first place.

I'm not sure I got what you mean, but as I undestood that it is not be a part of this fix, right?

clang/test/Analysis/initialization.c
102

Yes. I tryed. It works. Thanks! But I'll add a test case.

martong added inline comments.Oct 12 2021, 9:09 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

I think this should be part of this fix, but you don't have to do that iteration, there is a better thing for the job: getAnyInitializer.

What we need is to find that Decl that has the InitExpr attached. This may not be the canonical nor the most recent redecl.
Please see my code change suggestion below.

1763–1764
clang/test/Analysis/initialization.c
101–102

I'd like to see some more elaborate test cases. Notably

const int glob_arr3[];              // Incomplete array declaration
const int glob_arr3[4] = {1, 2, 3}; // Incomplete Array redeclaration
const int glob_arr3[];              // Incomplete array redeclaration

here neither the canonical nor the most recent decl have the initexpr.
And I think this is what @balazske tried to point out.

ASDenysPetrov added inline comments.Oct 12 2021, 9:55 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Thnx, I'll try this approach.

clang/test/Analysis/initialization.c
101–102

Exactly. I'll add this particular case, but I should mention that AST surprisingly shows the third redeclaration as ConstantArrayType with the extent. Thus, it works for the current fix.

|-VarDecl 0xc0725b0 <line:6:1, col:21> col:11 glob_arr3 'const int []'
|-VarDecl 0xc072700 prev 0xc0725b0 <line:7:1, col:34> col:11 glob_arr3 'const int [4]' cinit
| `-InitListExpr 0xc072830 <col:26, col:34> 'const int [4]'
|   |-array_filler: ImplicitValueInitExpr 0xc0728a8 <<invalid sloc>> 'const int'
|   |-IntegerLiteral 0xc072768 <col:27> 'int' 1
|   |-IntegerLiteral 0xc072788 <col:30> 'int' 2
|   `-IntegerLiteral 0xc0727a8 <col:33> 'int' 3
`-VarDecl 0xc0728e0 prev 0xc072700 <line:8:1, col:21> col:11 glob_arr3 'const int [4]'
steakhal added inline comments.Oct 12 2021, 10:24 AM
clang/test/Analysis/initialization.c
101–102

So, the redeclaration actually changed the type to ConstantArrayType. If so, you should probably mention this in the comment.

martong added inline comments.Oct 13 2021, 3:01 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Denys, I suppose we could replace the whole inner block with a function? Similarly to getSValFromStringLiteralByIndex from D107339 . That could greatly increase readability.

ASDenysPetrov added inline comments.Oct 15 2021, 6:02 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1763–1764

Make sense. I'll carry this block out.

clang/test/Analysis/initialization.c
101–102

OK, I'll mention.

Rebased on top of D106681.

NoQ accepted this revision.Oct 19 2021, 2:13 PM

Amazing, thanks a lot!

This revision is now accepted and ready to land.Oct 19 2021, 2:13 PM

Updated according to the latest suggestions.
@martong I think now this patch is way more simple and clear than before.

Amazing, thanks a lot!

Thank you, @NoQ, for the acceptance. I got some fresher update. And, of course in order to load it, we should accept a parent revision D106681 as well.

ASDenysPetrov updated this revision to Diff 380936.EditedOct 20 2021, 6:46 AM

Fixed redefinition in test file initialization.c.

steakhal added inline comments.Oct 20 2021, 9:10 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1649–1653

Wait. But if VD is null, you get a null-dereference.
But you already dereferenced VD multiple times, so it cannot be null.

Oh, but the getAnyInitializer() will overwrite it! That's a surprise.
TBH I would rather pass a fresh uninitialized pointer if you really need the exact decl which actually provided the initialized expression to make this behavior explicit.

That way, with a properly chosen name you could spare the NOTE comment as well.

martong accepted this revision.Oct 20 2021, 9:25 AM

Updated according to the latest suggestions.
@martong I think now this patch is way more simple and clear than before.

Thanks, indeed! LGTM (with nits about VD)!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1649–1653

Yes I agree, probably it is easier to follow the code if you make it explicit that VD is overwritten here.

ASDenysPetrov added inline comments.Oct 21 2021, 6:53 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1649–1653

Wait. But if VD is null, you get a null-dereference.

We have an assertion above.

Oh, but the getAnyInitializer() will overwrite it! That's a surprise.

So was for me :) I'll add a NOTE in the comment.

Rebased. Updated comment for obscure behaviour of getAnyInitializer function.

steakhal accepted this revision.Oct 25 2021, 1:38 AM

Minor nits. Aside from that just land it.
Thanks for the fix.

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

If the return value of getAnyInitializer() is null, then the value of VD will be actually preserved.

ASDenysPetrov added inline comments.Oct 25 2021, 4:52 AM
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
1654

I'll add the comment before the load.