This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] canonicalize special case of structure/pointer deref
ClosedPublic

Authored by vabridgers on Sep 28 2021, 6:54 AM.

Details

Summary

This simple change addresses a special case of structure/pointer
aliasing that produced different symbolvals, leading to false positives
during analysis.

The reproducer is as simple as this.

struct s {
  int v;
};

void foo(struct s *ps) {
  struct s ss = *ps;
  clang_analyzer_dump(ss.v); // reg_$1<int Element{SymRegion{reg_$0<struct s *ps>},0 S64b,struct s}.v>
  clang_analyzer_dump(ps->v); //reg_$3<int SymRegion{reg_$0<struct s *ps>}.v>
  clang_analyzer_eval(ss.v == ps->v); // UNKNOWN
}

Acks: Many thanks to @steakhal and @martong for the group debug session.

Event Timeline

vabridgers created this revision.Sep 28 2021, 6:54 AM
vabridgers requested review of this revision.Sep 28 2021, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 6:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Just an initial push to get this patch out there for review. I think this is pretty close. Thanks @steakhal and @martong for the debug session!

steakhal edited the summary of this revision. (Show Details)Sep 28 2021, 7:00 AM
steakhal added a reviewer: ASDenysPetrov.
steakhal edited the summary of this revision. (Show Details)
vabridgers planned changes to this revision.Sep 28 2021, 9:49 AM

update test case

Thanks Vince! Nice work!

Do you think it would be worth to have a test that checks the equality of a double pointer and a bi-dimensional array? Something like:

void struct_pointer_canon(struct s **ps) {
  struct s ss = **ps;
  clang_analyzer_eval(&(ps[0][0].v) == &((*ps)->v)); // expected-warning{{TRUE}}
}
martong accepted this revision.Sep 28 2021, 12:52 PM
This revision is now accepted and ready to land.Sep 28 2021, 12:52 PM

@martong, Thanks for yes for sure! I'll post an update soon with a few minor test improvements.

improve test cases per @martong

nits

clang/test/Analysis/ptr-arith.c
337

It's probably unnecessary.

340

I would probably put this at the top of the file.

345

You should not hardcode the internal counter in the tests.
You can use regexp matching to workaround the issue. Here is an example:
// expected-warning-re@-2 {{reg_${{[[:digit:]]+}}<int v>}}

@-2 denotes that we expect a warning two lines above.

updates per @steakhal comments

vabridgers marked 3 inline comments as done.Sep 29 2021, 1:52 AM

Thanks @steakhal and @martong for the comments! The patch has been updated.

I think we need to get canonical types.
Check, please, aliased types:

struct s {
  int v;
};
using T1 = s;
typedef s T2;
void foo(T1 *ps) {
  T2 ss = *ps;
  ...
}

update per comments from Denys

@ASDenysPetrov , thank you for the comment. I added a test case per your suggestion.

'using' is the same as 'typedef' AFAIK.
So, you could simply use only typedefs and implement the test in the c test file. Seeing all the tests close together would aid readability IMO.
WDYT Denys? Btw does the SVval::getType return a canonical type in all cases?

ASDenysPetrov added a comment.EditedOct 1 2021, 1:51 AM

'using' is the same as 'typedef' AFAIK.
So, you could simply use only typedefs and implement the test in the c test file. Seeing all the tests close together would aid readability IMO.

You're right. I'm OK with typedef.

WDYT Denys? Btw does the SVval::getType return a canonical type in all cases?

SVal::getType returns a QualType which can be aliased and cv-qualified. So, it may mismatch in comparison BT->getPointeeType() == elementType. That is my concern.

vabridgers updated this revision to Diff 376450.Oct 1 2021, 1:51 AM

added typedef case to C file for comparison, discussion

vabridgers updated this revision to Diff 376452.Oct 1 2021, 1:58 AM

move test case from cpp file to c file

vabridgers updated this revision to Diff 376453.Oct 1 2021, 2:09 AM

add const test case

I moved the test cases from ptr-arith.cpp to ptr-arith - using to typedef, and created an extra test case that uses const. I believe this may address all concerns discussed thus far? Please let me know if we need anything more and I'll get it done. Best!

WDYT Denys? Btw does the SVval::getType return a canonical type in all cases?

SVal::getType returns a QualType which can be aliased and cv-qualified. So, it may mismatch in comparison BT->getPointeeType() == elementType. That is my concern.

I thought that SVal::getType should return an already canonical QualType. If it doesn't do that we would need to do canonicalization at each callsite, which is less than ideal IMO.
If it's not yet canonical, we should probably canonicalize within the getType() function probably. WDYT?


I moved the test cases from ptr-arith.cpp to ptr-arith - using to typedef, and created an extra test case that uses const. I believe this may address all concerns discussed thus far? Please let me know if we need anything more and I'll get it done. Best!

I think your change is good, but we need to sort some things out before moving forward with that.

I thought that SVal::getType should return an already canonical QualType. If it doesn't do that we would need to do canonicalization at each callsite, which is less than ideal IMO.
If it's not yet canonical, we should probably canonicalize within the getType() function probably. WDYT?

I believe that it returns non-canonical type. Yes, we shall get canonical versions separately for most cases in practice. This is not ideal but it would be worse to return canonical types at once along with loss of extra information.

I thought that SVal::getType should return an already canonical QualType. If it doesn't do that we would need to do canonicalization at each callsite, which is less than ideal IMO.
If it's not yet canonical, we should probably canonicalize within the getType() function probably. WDYT?

I believe that it returns non-canonical type. Yes, we shall get canonical versions separately for most cases in practice. This is not ideal but it would be worse to return canonical types at once along with loss of extra information.

Okay, if this is the case, we will need to use getCanonicalType() on both BT and elementType.
Vince, could you update your change, just to make sure it will work in the future, even if the test doesn't show much difference with the current behavior?
Note that you cannot call getCanonicalType() on a null-type.

vabridgers updated this revision to Diff 376526.Oct 1 2021, 7:29 AM

Use canonical types for comparison

vabridgers planned changes to this revision.Oct 5 2021, 9:53 AM

I'm refactoring the code change a bit. I'll push another update soon. Thanks for the comments.

Refactor compare a little bit

This revision is now accepted and ready to land.Oct 5 2021, 11:13 AM
steakhal accepted this revision.Oct 6 2021, 2:06 AM

It looks great.
Thanks Vince!