This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Get direct binding for specific punned case
ClosedPublic

Authored by vabridgers on Apr 24 2022, 12:14 PM.

Details

Summary

Region store was not able to see through this case to the actual
initialized value of STRUCT ff. This change addresses this case by
getting the direct binding. This was found and debugged in a downstream
compiler, with debug guidance from @steakhal. A positive and negative
test case is added.

The specific case where this issue was exposed.

typedef struct {
  int a:1;
  int b[2];
} STRUCT;

int main() {
  STRUCT ff = {0};
  STRUCT* pff = &ff;
  int a = ((int)pff + 1);
  return a;
}

Diff Detail

Event Timeline

vabridgers created this revision.Apr 24 2022, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 12:14 PM
vabridgers requested review of this revision.Apr 24 2022, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 12:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

clang-format

steakhal accepted this revision.Apr 25 2022, 12:11 AM

Looks good.
Wait for an other accept.

clang/test/Analysis/array-struct-region.c
362–366 ↗(On Diff #424796)
void array_struct_bitfield_1() {
  BITFIELD_CAST ff = {0};
  BITFIELD_CAST *pff = &ff;
  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
  ff.b[0] = 3;
  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
}
This revision is now accepted and ready to land.Apr 25 2022, 12:11 AM
vabridgers added inline comments.Apr 25 2022, 1:30 AM
clang/test/Analysis/array-struct-region.c
362–366 ↗(On Diff #424796)

Good suggestion, thanks. I'll add this and wait for another accept.

update LIT per comments from @steakhal

vabridgers marked an inline comment as done.Apr 25 2022, 1:37 AM

polite ping to @NoQ and/or @martong. @steakhal is ok with this change, are you ok if I land this? Thanks!

Can we have a test for this, got idea from here (https://stackoverflow.com/questions/4129961/how-is-the-size-of-a-struct-with-bit-fields-determined-measured)

typedef struct
{
        unsigned int a:1;
        unsigned int x:31;
        unsigned int c:1;
        int b[2];
} mystruct;
...
ff.b[0] = 3;
clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???
clang/test/Analysis/array-struct-region.c
1 ↗(On Diff #424844)

Should we pin the target, shouldn't we?

365 ↗(On Diff #424844)

My gut feeling is that we should pin the target for these arithmetics.

Can we have a test for this, got idea from here (https://stackoverflow.com/questions/4129961/how-is-the-size-of-a-struct-with-bit-fields-determined-measured)

typedef struct
{
        unsigned int a:1;
        unsigned int x:31;
        unsigned int c:1;
        int b[2];
} mystruct;
...
ff.b[0] = 3;
clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???

Generally, you are right. But in this case, we are talking about a *single bit* bitfield.
That bitfield cannot span across multiple unsigned objects. And int is supposed to be at least one byte large, hence there is plenty of room for an additional CHAR_BIT - 1 bits along with this one and we would be still portable.

clang/test/Analysis/array-struct-region.c
1 ↗(On Diff #424844)

There is no need for that.
The sizeof(int) might change, but the operator+ will accommodate for that in the pointer arithmetic. And the field after bitfields is by default aligned to its preferred alignment.

Thanks for the comments! Per our discussion, I'll create a different test case with a pinned target and add the additional test case to be sure we see expected behavior. Best!

Address comments from @martong

martong accepted this revision.May 5 2022, 12:16 AM

LGTM! Thanks Vince!

clang/test/Analysis/array-punned-region.c
39
vabridgers updated this revision to Diff 427246.May 5 2022, 2:47 AM

fix a stupid leftover cut/paste mistake in the test case :/

vabridgers marked an inline comment as done.May 5 2022, 2:48 AM

Thanks @martong. I fixed the "typo" :/, landing. Best!

This revision was landed with ongoing or failed builds.May 5 2022, 2:54 AM
This revision was automatically updated to reflect the committed changes.
isuckatcs added inline comments.
clang/test/Analysis/array-punned-region.c
24

@steakhal @martong @NoQ

Isn't this actually a false positive here?

(int *)pff + 2 points to ff.b[1], which is initialized to 0.

https://godbolt.org/z/Gh8a4aMe8