This is an archive of the discontinued LLVM Phabricator instance.

[clang] Rework IsTailPaddedMemberArray into isFlexibleArrayMemberExpr

Authored by serge-sans-paille on Sep 1 2022, 6:18 AM.



This fixes a bunch of FIXME within IsTailPaddedMemberArray related code.

As a side effect, this now also triggers a warning when trying to access a
"struct hack" field with an index that cannot be represented in associated address space.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 6:18 AM
serge-sans-paille requested review of this revision.Sep 1 2022, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 6:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
msebor added a subscriber: msebor.Sep 13 2022, 9:39 AM
msebor added inline comments.

I'm not familiar with this code but I'm guessing that at most one of the if conditionals is true, and if it's the first one then the second dyn_cast will fail. Should the if guarding this assignment then be an else if?

aaron.ballman added inline comments.Sep 15 2022, 10:07 AM

depending on <ins>the value of</ins> -fstrict-flex-array <del>value</del><ins>.</ins>

(Sorry, the Suggest Edits button seems to be unavailable)


While this code is moving around from elsewhere, +1, this should use an else if. Also, you should use const auto * for the declarations since the type is explicitly spelled out in the initializers.


You can drop the parens.

Take reviews into account

Found a few more things while trying to convince myself this really is NFC, and I don't think it is. If you agree, then I think we should have some additional test coverage to show the behavioral changes.


Missed this before, but drop the top-level const or remove the local and just use the call directly in the one place the value is needed.


This seems like a functional change....


I think the parens helped to add clarity here (also, there's probably a warning generated about mixed || and && from this too, isn't there?)


...because now we can get into this block...


... which means we can now hit this diagnostic.

serge-sans-paille edited the summary of this revision. (Show Details)

Address minor nits, plus add a test case that showcases the extra (legitimate!) warning we now get when accessing a "struct hack" field with an index that cannot be represented in associated address space.

Kudos to @aaron.ballman for spotting this one!

As a comparison, current behavior for the same test case: (only the flexible array member is flagged with a warning, the struct hacks are ignored)

msebor added inline comments.Sep 20 2022, 2:20 PM

There's a difference between the sizes of fam1 and fam that makes accesses to the four leading elements of fam1.tail strictly in bounds, while no access to either fam.tail or fam0.tail is (sizeof fam is the same as sizeof int while sizeof fam1 is equal to sizeof (int[2]) on common targets). It would be helpful to capture that difference in the tests, both for the warning and for __builtin_object_size.

There should also be a difference between accessing elements of an object of an initialized struct with a flexible array member (i.e., one whose size is known) and those of an object that's only declared but that's defined in some other translation unit. Since the size of the object is determined by its initializer, it should be reflected in __builtin_object_size and accesses to it checked by -Warray-bounds. The size of the latter object is unknown it must be assumed to be PTRDIFF_MAX - sizeof (int) - 1. It would also be helpful to add tests for these cases.

As far as I can see, none of these cases seems to be handled quite right on trunk. For example, the size of s below should be 8 but Clang evaluates __builtin_object_size(&s, N) to 4, without diagnosing any past-the-end accesses to s.a:

struct S {
  int n;
  char a[];
} s = { 1, { 2, 3, 4, 5 } };
msebor added inline comments.Sep 20 2022, 3:37 PM

I opened PR #57860 to better show what I mean.


Agreed. I suggest we discuss that in this PR, while me merge this "code cleanup with enhancement", if @aaron.ballman agrees :-)

aaron.ballman accepted this revision.Sep 21 2022, 9:55 AM

LGTM, though this should probably have a release note for it (we can augment the release note when we make further changes in this area).


+1 -- there are bugs there to be fixed, but handling that separately from this cleanup seems like it will be easier to review and discuss.

This revision is now accepted and ready to land.Sep 21 2022, 9:55 AM