[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays

[serge-sans-paille]

Summary

Some code [0] consider that trailing arrays are flexible, whatever their size.
Support for these legacy code has been introduced in
f8f632498307d22e10fab0704548b270b15f1e1e but it prevents evaluation of
builtin_object_size and builtin_dynamic_object_size in some legit cases.

Introduce -fstrict-flex-arrays=<n> to have stricter conformance when it is
desirable.

n = 0: current behavior, any trailing array member is a flexible array. The default.
n = 1: any trailing array member of undefined, 0 or 1 size is a flexible array member
n = 2: any trailing array member of undefined or 0 size is a flexible array member

Similar patch for gcc discuss here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836

[0] https://docs.freebsd.org/en/books/developers-handbook/sockets/#sockets-essential-functions

[serge-sans-paille]

Note: I named the option -fstrict-flex-arrays and not -fstrict-flex-array because we already have -fstrict-enums.

[xbolva00]

clang/docs/ClangCommandLineReference.rst

2624

arrays

[serge-sans-paille]

Fix typo

[efriedma]

Probably IsTailPaddedMemberArray() in SemaChecking.cpp and isFlexibleArrayMemberExpr() in CGExpr.cpp should also check for this flag. Not sure if there's anything else that does similar checks; the static analyzer has its own flag consider-single-element-arrays-as-flexible-array-members, but I think that's disabled by default anyway.

I'm a little concerned about the premise of this, though. See https://github.com/llvm/llvm-project/issues/29694 for why we relaxed this check in the first place. I mean, the Linux kernel itself can maybe ensure it isn't doing anything silly, but most code has to deal with system headers, which are apparently broken. So this option is a trap for most code.

[kees]

Thanks for working on this!

Doing test builds with the Linux kernel correctly detects a number of trailing arrays that were being treated as flexible arrays (and need to be fixed in the kernel). This is exactly what was expected and wanted. :)

clang/include/clang/Basic/LangOptions.def

425

I think this option should likely also affect the logic in -fsanitize=bounds too, though I think that could be a separate change. Fixing __bos is more important. :)

clang/test/CodeGen/object-size-flex-array.c

27

Perhaps add some additional tests for "not a trailing array" here too, just for completeness?

typedef struct {
  double c[0];
  float f;
} fooM0_t;

typedef struct {
  double c[1];
  float f;
} fooM1_t;

typedef struct {
  double c[2];
  float f;
} fooM2_t;

34

Shouldn't this explicitly return -1?

[kees]

In D126864#3556262, @efriedma wrote:

I'm a little concerned about the premise of this, though. See https://github.com/llvm/llvm-project/issues/29694 for why we relaxed this check in the first place. I mean, the Linux kernel itself can maybe ensure it isn't doing anything silly, but most code has to deal with system headers, which are apparently broken. So this option is a trap for most code.

Fixing system headers will likely come after this lands. Code bases that can use it (e.g. Linux kernel) will pave the way. But yes, totally agreed: it cannot be default-enabled.

As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of struct sockaddr needs to change. :) The Linux kernel has played games like that before, but we've been removing them all for saner implementations (which is why -fstrict-flex-arrays is desired: flushing out any remaining weird spots).

[efriedma]

As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of struct sockaddr needs to change. :)

Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise.

[kees]

In D126864#3564519, @efriedma wrote:

Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise.

Hm, maybe? It wonder if that's more confusing, as code would need to do really careful management of allocation sizes.

struct foo {
    u32 len;
#ifndef STRICT_FLEX_ARRAYS
    u32 array[14];
#else
    u32 array[];
#endif
};

sizeof(struct foo) will change. Or, even more ugly:

struct foo {
  union {
    struct {
      u32 len_old;
      u32 array_old[14];
    };
    struct {
      u32 len;
      u32 array[];
    };
  };
};

But then sizeof(struct foo) stays the same.

(FWIW, the kernel is effectively doing the latter without any define.)

[nickdesaulniers]

In D126864#3564519, @efriedma wrote:

As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of struct sockaddr needs to change. :)

Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise.

Worst case, we don't need this at least for the kernel.

The kernel has machinery to test if a command line flag is available, and if so, set a preprocessor define via -D command line invocation which can be used as a guard in kernel sources.

Would this preprocessor define be handy for code outside the kernel? /me shrug

But for now we don't need it.

@kees maybe we should think about what would be needed for toolchains that don't yet support -fstrict-flex-arrays in kernel sources? Does this become a toolchain portability issue for older released toolchains we still intend to support?

[kees]

In D126864#3567647, @nickdesaulniers wrote:

@kees maybe we should think about what would be needed for toolchains that don't yet support -fstrict-flex-arrays in kernel sources? Does this become a toolchain portability issue for older released toolchains we still intend to support?

Gaining this for the kernel is just to make sure all the trailing arrays are actually getting found and handled. The kernel will handle this fine either way. (It already has to treat UAPI 1-element array conversions very carefully...)

[serge-sans-paille]

Take into account @efriedma and @kees review, plus adding a bunch of extra test case

[msebor]

Quite a bit more code treats one-element trailing arrays as flexible array members than larger arrays, and may not be able to change (GCC is one example). I wonder if it would be worthwhile to make it possible for code to specify that mode (e.g., by adding two options, or by having the new option accept an argument specifying the cutoff at which a trailing array is not considered a flexible array member).

Also, for better compatibility it would help to add the corresponding option to GCC.

clang/docs/ReleaseNotes.rst

71–75

This sentence looks mangled. It should probably read something like

Clang now supports the -fstrict-flex-arrays option to avoid treating trailing array members with a specified bound as flexible array members. The option yields more accurate __builtin_object_size results in most cases but may be overly conservative for some legacy code.

(The technical term is flexible array member.)

If the option also affects __builtin_dynamic_object_size it might be worth mentioning that as well, if only to highlight the latter built-in since it's more powerful.

[serge-sans-paille]

Update changelog entry

[serge-sans-paille]

Cleanup testcase

[serge-sans-paille]

clang/test/CodeGen/object-size-flex-array.c

34

There's still the possibility that llvm folds that to an actual constant if it can track the allocation site (not the case for that particular function, but I think that's not for clang to decide that)

[serge-sans-paille]

Looks good on my side, waiting for feedback ;-)

[kees]

Are the presubmit build failures unrelated?

[nickdesaulniers]

If the GCC developers split this into two distinct flags, should we land something we're just going to have to turn around and modify to match the new flags/semantics they've created?

[kees]

In D126864#3582360, @nickdesaulniers wrote:

If the GCC developers split this into two distinct flags, should we land something we're just going to have to turn around and modify to match the new flags/semantics they've created?

I'm cautious about waiting on theoretical design choices. They appear to agree that a "fully strict" mode should exist, and that's what -fstrict-flex-arrays here gets us. I'd rather get this into Clang now so I can start working through all the new cases it finds instead of waiting for bikeshedding to end. If Clang needs to rename the option in a few months, I think that's okay. But I'll leave it up to @serge-sans-paille

[efriedma]

Maybe we should document that zero-size arrays aren't treated as flexible arrays, since that's what the zero-size array extension was originally designed for.

Looks fine otherwise.

[serge-sans-paille]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 does toward -fstrict-flex-arrays=<n> with

• n=0 ⇒ -fno-strict-flex-arrays, current state (the default)
• n=1 ⇒ only consider [ 0], [1] and [ ] as flex array member
• n=2 ⇒ only consider [ 0] and [ ] as flex array member
• n=3 ⇒ only consider [ ] as flex array member

I personnally like that approach, and I'd rather land that implementation.

[kees]

In D126864#3584536, @serge-sans-paille wrote:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 does toward -fstrict-flex-arrays=<n> with

• n=0 ⇒ -fno-strict-flex-arrays, current state (the default)
• n=1 ⇒ only consider [ 0], [1} and [ ] as flex array member
• n=2 ⇒ only consider [ 0] and [ ] as flex array member
• n=3 ⇒ only consider [ ] as flex array member

I personnally like that approach, and I'd rather land that implementation.

Sounds good to me!

[serge-sans-paille]

Update option and code to handle several level of conformance

Syndicate code across clang codebase to a single point

update test case accordingly

[serge-sans-paille]

@kees does the new version looks good to you?

[kees]

In D126864#3598257, @serge-sans-paille wrote:

@kees does the new version looks good to you?

Hi, yes, this is working as expected in my kernel builds. Yay! :)

[dblaikie]

Looks like this got committed in 886715af962de2c92fac4bd37104450345711e4a though it was missing the Differential Revision tag to allow phab (or humans) to connect the commit with the review.

In any case, this seems to be causing a UBSan (UndefinedBehaviorSanitizer: out-of-bounds-index third_party/python_runtime/v3_9/Objects/codeobject.c:54:34 ) regression in Python 3.9 at Google - I don't have a readily shareable standalone reproduction just now - but would you consider reverting while this is investigated further?

(@cmtice)

[efriedma]

I suspect the refactoring in the latest version of the patch accidentally made UBSan a bit more strict by default.

[cmtice]

I did some testing and verified that this commit is causing some of the Sanitizer issues we are seeing. Is there any chance of a revert?

[MaskRay]

The CPython usage is a real UB: https://github.com/python/cpython/issues/94250

In D126864#3609497, @efriedma wrote:

I suspect the refactoring in the latest version of the patch accidentally made UBSan a bit more strict by default.

Yes, the patch accidentally dropped a -fsanitize=array-bounds workaround for size-1 array as the last member of a structure. I added it back in 572b08790a69f955ae0cbb1b4a7d4a215f15dad9

[serge-sans-paille]

Thanks @MaskRay for the quick patch!

[xbolva00]

-fsanitize=array-bounds workaround for size-1 array as the last member of a structure

Could you provide option for that (to enable stricker bound checks introduced with this patch) ?

[MaskRay]

In D126864#3609965, @xbolva00 wrote:

-fsanitize=array-bounds workaround for size-1 array as the last member of a structure

Could you provide option for that (to enable stricker bound checks introduced with this patch) ?

My commit just restored the previous -fsanitize=array-bounds behavior (as if the default is -fstrict-flex-arrays=1, different from Sema).
-fstrict-flex-arrays=2 or above can make -fsanitize=array-bounds stricter.

[sberg]

see https://reviews.llvm.org/D128643 "[clang] -fsanitize=array-bounds: treat all trailing size-1 arrays as flexible" for another regression

[vitalybuka]

This bot is broken by this patch
https://lab.llvm.org/buildbot/#/builders/85/builds/8881

[vitalybuka]

Reverted with cdfa15da94f06289dcf86173d18b6627f92ac403 (inlcuding 572b08790a69f955ae0cbb1b4a7d4a215f15dad9)

[vitalybuka]

exactly as reverted revision
886715af962de2c92fac4bd37104450345711e4a + 572b08790a69f955ae0cbb1b4a7d4a215f15dad9

[vitalybuka]

restore blank line

[MaskRay]

clang/include/clang/Driver/Options.td

1135

clang/test/CodeGen/bounds-checking-fma.c

1 ↗

(On Diff #440396)

I realized that I made a typo. It should be fam instead of fma....

I precomitted a test for the default case. You may add -fstrict-flex-arrays={1,2} lines on top of it.

clang/test/CodeGen/object-size-flex-array.c

100

[sberg]

In D126864#3612149, @sberg wrote:

see https://reviews.llvm.org/D128643 "[clang] -fsanitize=array-bounds: treat all trailing size-1 arrays as flexible" for another regression

For the record (now that the original commit has been reverted anyway for now):

Besides the above regression for -fsanitize=array-bounds and macro'ized array size in HarfBuzz, I also ran into yet another regression when Firebird is built with -fsanitize=array-bounds: That project, in https://github.com/FirebirdSQL/firebird/blob/master/src/lock/lock_proto.h uses a trailing member

	srq lhb_hash[1];			// Hash table

as a flexible array, but declared in a

struct lhb : public Firebird::MemoryHeader

that is not a standard-layout class (because the Firebird::MemoryHeader base class also declares non-static data members). And isFlexibleArrayMember returns false if the surrounding class is not a standard-layout class.

In the end, what brought back a successful -fsanitize=array-bounds LibreOffice (which bundles those HarfBuzz and Firebird, among others) for me was to exclude the whole set of blocks

// Don't consider sizes resulting from macro expansions or template argument
// substitution to form C89 tail-padded arrays.

TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
while (TInfo) {
  TypeLoc TL = TInfo->getTypeLoc();
  // Look through typedefs.
  if (TypedefTypeLoc TTL = TL.getAs<TypedefTypeLoc>()) {
    const TypedefNameDecl *TDL = TTL.getTypedefNameDecl();
    TInfo = TDL->getTypeSourceInfo();
    continue;
  }
  if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
    const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
    if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
      return false;
  }
  break;
}

const ObjCInterfaceDecl *ID =
    dyn_cast<ObjCInterfaceDecl>(FD->getDeclContext());
const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
if (RD) {
  if (RD->isUnion())
    return false;
  if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
    if (!CRD->isStandardLayout())
      return false;
  }
} else if (!ID)
  return false;

(by means of an additional bool parameter) when isFlexibleArrayMember is called from getArrayIndexingBound (in clang/lib/CodeGen/CGExpr.cpp).

[MaskRay]

In D126864#3614235, @sberg wrote:

In D126864#3612149, @sberg wrote:

see https://reviews.llvm.org/D128643 "[clang] -fsanitize=array-bounds: treat all trailing size-1 arrays as flexible" for another regression

For the record (now that the original commit has been reverted anyway for now):

Besides the above regression for -fsanitize=array-bounds and macro'ized array size in HarfBuzz, I also ran into yet another regression when Firebird is built with -fsanitize=array-bounds: That project, in https://github.com/FirebirdSQL/firebird/blob/master/src/lock/lock_proto.h uses a trailing member

	srq lhb_hash[1];			// Hash table

as a flexible array, but declared in a

struct lhb : public Firebird::MemoryHeader

that is not a standard-layout class (because the Firebird::MemoryHeader base class also declares non-static data members). And isFlexibleArrayMember returns false if the surrounding class is not a standard-layout class.

In the end, what brought back a successful -fsanitize=array-bounds LibreOffice (which bundles those HarfBuzz and Firebird, among others) for me was to exclude the whole set of blocks

// Don't consider sizes resulting from macro expansions or template argument
// substitution to form C89 tail-padded arrays.

TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
while (TInfo) {
  TypeLoc TL = TInfo->getTypeLoc();
  // Look through typedefs.
  if (TypedefTypeLoc TTL = TL.getAs<TypedefTypeLoc>()) {
    const TypedefNameDecl *TDL = TTL.getTypedefNameDecl();
    TInfo = TDL->getTypeSourceInfo();
    continue;
  }
  if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
    const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
    if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
      return false;
  }
  break;
}

const ObjCInterfaceDecl *ID =
    dyn_cast<ObjCInterfaceDecl>(FD->getDeclContext());
const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
if (RD) {
  if (RD->isUnion())
    return false;
  if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
    if (!CRD->isStandardLayout())
      return false;
  }
} else if (!ID)
  return false;

(by means of an additional bool parameter) when isFlexibleArrayMember is called from getArrayIndexingBound (in clang/lib/CodeGen/CGExpr.cpp).

Oh, I did not see this when pushing a test efd90ffbfc427ad4c4675ac1fcae9d53cc7f1322 . Consider adding additional tests in clang/test/CodeGen/bounds-checking-fam.c.
It's worth bringing up such issue to the project. GCC and Clang have been working around them so far but they should eventually be fixed.
For CPython I try to be loud on https://github.com/python/cpython/issues/84301

[serge-sans-paille]

GCC and Clang don't have the same behavior wrt. macros-as-abound and standard-layout-requirement, see https://godbolt.org/z/3vc4TcTYz
I'm fine with keeping the CLang behavior, but do we want to keep it only for level=0, and drop it for higher level (this would look odd to me).

I'd say that we keep it for level 0,1,2, but then I think we should *not* make it an option of the isFlexibleArrayMember method. That would allow for inconsistent behavior wrt. FAM across the codebase and I'm not a fan of it.

Thoughts?

[sberg]

In D126864#3614297, @MaskRay wrote:

Oh, I did not see this when pushing a test efd90ffbfc427ad4c4675ac1fcae9d53cc7f1322 . Consider adding additional tests in clang/test/CodeGen/bounds-checking-fam.c.

see https://reviews.llvm.org/D128783 "Check for more -fsanitize=array-bounds regressions"

It's worth bringing up such issue to the project. GCC and Clang have been working around them so far but they should eventually be fixed.

All the issues I encountered were in C++ code, and I'm not sure replacing one non-standard behavior (treating trailing length-one arrays as flexible) with another (C flexible array members) is something I feel confident to suggest to those projects.

[serge-sans-paille]

Code updated to take into account two situations:

• size resulting from macro expansion. Previous behavior was inconsistent in that situation. I chose to consider that int a[1] and int a[N] where N is a macro definition both are FAM. That's GCC behavior too. CCing @chandlerc because he introduced that feature, probably because that could make an array a FAM just based on preprocessor flag (?)

• FAM within non-standard layout C++ object. GCC allows that behavior and clang used to have an inconsistent approach here. I'd like to stick to GCC behavior.

Note that we still diverge from GCC behavior for int a[N] when N results from template expansion. I could revert that too.

[msebor]

In D126864#3616524, @serge-sans-paille wrote:

GCC and Clang don't have the same behavior wrt. macros-as-abound and standard-layout-requirement, see https://godbolt.org/z/3vc4TcTYz
I'm fine with keeping the CLang behavior, but do we want to keep it only for level=0, and drop it for higher level (this would look odd to me).

If you're referring to the warning, GCC needs -O2 to issue most instances of -Warray-bounds. The warning also treats one-element trailing arrays as flexible array members, so it's intentionally silent about the test case. GCC should update the warning to reflect the -fstrict-flex-arrays=N level. (Macros are long expanded by the time the code reaches the middle end so they don't come into play, except perhaps when they come from system headers.)

[serge-sans-paille]

Do *not* try to syndicate code across different location. Alhtough that would be benefitial to the project, it also makes this patch too complex and controversial because it would imply regression on some behaviors.

Instead, focus on compatibility with existing tests, and refine existing behavior wrt. bound size.

[jyknight]

As I commented on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836#c32,

It doesn't make sense to have a mode in which int array[0] is accepted but is not a flex array.
Either that should be a compilation error (as the standard specifies), or it should be a flex array. Accepting it as an extension but having it do the wrong thing is not useful or helpful.
Note that Clang has a dedicated warning flag for zero-length arrays: -Wzero-length-array, so anyone who wants to prohibit them may use -Werror=zero-length-array. It would be helpful for GCC could follow suit there.

The -fstrict-flex-arrays=3 option should be removed.

[jyknight]

clang/docs/ClangCommandLineReference.rst

2626

Docs should also mention what the default -fno-strict-flex-arrays means -- that ALL sizes of trailing arrays are considered flexible array members. (I'm amazed that's the rule, and I never knew it. I always thought the special casing for FAMs was restricted to sizes 0 and 1!)

Also, since apparently different parts of the compiler have been (and will now continue to) use different default behaviors, may want to document that as well. I'm sure I don't know what the rules actually are intended to be here. E.g. that a macro-expansion of the size arg disables the special-behavior for [1] is extremely surprising!

clang/lib/Sema/SemaChecking.cpp

15804 ↗

(On Diff #442592)

Presumably the size-zero/unsized cases are already being taken care of elsewhere in the code? I find it hard to believe we are currently emitting diagnostics for size-0 FAM which we don't emit for size-1 FAM?

clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp

9 ↗

(On Diff #442592)

Except we actually _do_ know the bounds of the full-object and ought to be able to warn on this code anyhow...

Better to have the test function accept a pointer, so that's not a conflating issue?

[serge-sans-paille]

clang/docs/ClangCommandLineReference.rst

2626

it is worse than that: for some checks, any size is valid for FAM, but not for alls. For some checks, macro expansion prohibits FAM, but not for all, etc, etc. I don't want to document that behavior, because it is too specific to each pass. My plan is

1. land -fstrict-flex-array support
2. launch a thread on the ugly situation we put ourselves in, and extract a decision for each case
3. support and document an homogeneous behavior across passes.
4. syndicate code across passes

clang/lib/Sema/SemaChecking.cpp

15804 ↗

(On Diff #442592)

correct

15969 ↗

(On Diff #442592)

And here

15973 ↗

(On Diff #442592)

Handled here

clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp

9 ↗

(On Diff #442592)

Correct

[serge-sans-paille]

Take into account @jyknight review.

[serge-sans-paille]

Remove debug code

[jyknight]

clang/lib/CodeGen/CGExpr.cpp

892 ↗

(On Diff #442692)

Similar to SemaChecking below, could use a comment like:

FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing arrays to be treated as flexible-array-members, we still emit ubsan checks as if they are not. Pending further discussion...

906 ↗

(On Diff #442692)

I believe this bit is incorrect -- it should just go back to 'return true;'. The StrictFlexArraysLevel check above already eliminates the cases we want to eliminate (size==1 in strictness-level 2.)

clang/lib/Sema/SemaChecking.cpp

15804 ↗

(On Diff #442592)

The FIXME comment isn't correct, since only nonzero sizes ever reach this function. The code can be simplified too. Also -- there is another FIXME that should be here, regarding the behavior of size>1 FAMs.

I suggest (with rewrapped comment of course):

if (!ND) return false;
// FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing arrays to be treated as flexible-array-members, we still emit diagnostics as if they are not. Pending further discussion...
if (StrictFlexArraysLevel >= 2 || Size != 1) return false;`

15969 ↗

(On Diff #442592)

Looks like actually the int x[] case is handled with the large "IsUnboundedArray" condition above not here...

And, actually, all of that code to generate warnings for larger-than-addrspace offsets OUGHT to be getting used for int x[0] and int x[1] flexible arrays, too. Needs another FIXME for that...

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

792 ↗

(On Diff #442692)

Yuk, another place that is weird and doesn't handle StrictFlexArrays as expected...

[serge-sans-paille]

Take reviews into account, basically spreading a lot of FIXME across the codebase :'(

[serge-sans-paille]

@kees are you ok with current state?

clang/lib/CodeGen/CGExpr.cpp

906 ↗

(On Diff #442692)

Well, if we are in strictness-level 2, with an undefined size or size = 0, we can still reach that path, and don't want to return 'true' because FAM in union are in invalid per the standard.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

792 ↗

(On Diff #442692)

I've been able to slightly move that check below, without impacting test suite. Also added a FIXME because Opts.ShouldConsiderSingleElementArraysAsFlexibleArrayMembers looks redundant with -fstrict-flex-arrays

[jyknight]

clang/lib/CodeGen/CGExpr.cpp

906 ↗

(On Diff #442692)

Yes, we can reach this path, which is why the change is incorrect. We should not be changing the FAMness of undefined size, or size == 0, in any of the modes. To be more specific --

union X { int x[0]; }; should still be a FAM in all strictness modes. (if you don't want zero-length-arrays, use -Werror=zero-length-array).

For union X { int x[]; };: this ought to be a compiler error. It's likely just an oversight that we currently accept it; I'd be OK with a (separate) patch to fix that. (GCC emits an error, so there's unlikely to be compatibility issues with such a change.)

clang/lib/Sema/SemaChecking.cpp

15972 ↗

(On Diff #443185)

What I meant to say is that both the IsTailPaddedMemberArray call, and this code, should be moved up above, in order to set IsUnboundedArray to true, so we will get the checks from that block.

(But doesn't need to be in this patch, fine to just leave a comment to that effect).

15804 ↗

(On Diff #442592)

You didn't like the code change I suggested there?

15969 ↗

(On Diff #442592)

Is it redundant? This check is for an _incomplete_ type as the array element type, which doesn't seem directly related to unbounded array sizes. (though it also prevents knowing the size of the array)

[kees]

clang/lib/CodeGen/CGExpr.cpp

906 ↗

(On Diff #442692)

union X { int x[0]; }; should still be a FAM in all strictness modes. (if you don't want zero-length-arrays, use -Werror=zero-length-array).

The Linux kernel cannot use -Wzero-length-array because we have cases of userspace APIs being stuck with them. (i.e. they are part of the struct declaration, even though the kernel code doesn't use them.) For example:

In file included from ../kernel/bounds.c:13:
In file included from ../include/linux/log2.h:12:
In file included from ../include/linux/bitops.h:9:
In file included from ../include/uapi/linux/kernel.h:5:
../include/uapi/linux/sysinfo.h:22:10: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
        char _f[20-2*sizeof(__kernel_ulong_t)-sizeof(__u32)];   /* Padding: libc5 uses this.. */
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[kees]

In D126864#3641939, @serge-sans-paille wrote:

@kees are you ok with current state?

Build tests are still looking correct on my end. Thanks!

[kees]

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and sizeof([]) == error, they are being treated differently already by the compiler causing bugs in Linux. The kernel must still have a way to reject the _use_ of a [0] array. We cannot reject _declaration_ of them due to userspace API.

[jyknight]

In D126864#3645994, @kees wrote:

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and sizeof([]) == error, they are being treated differently already by the compiler causing bugs in Linux. The kernel must still have a way to reject the _use_ of a [0] array. We cannot reject _declaration_ of them due to userspace API.

Looks like the linux kernel is currently chock-full of zero-length-arrays which are actually intended/required to work as flexible array members. Do you have a pending patch to convert all of them to [] which should be? If you're saying you absolutely need this mode -- which I still believe would be nonsensical to provide -- I'd like to see the set of concrete examples you cannot otherwise deal with.

clang/lib/CodeGen/CGExpr.cpp

906 ↗

(On Diff #442692)

ISTM you can simply remove this field on 64-bit platforms, without changing the ABI. If you're worried about API breakage, for anything that might use the field-name _f (?), you could make it visible only to user-space or

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wzero-length-array"
...
#pragma GCC diagnostic pop

if really needed.

[serge-sans-paille]

I'd like to see the set of concrete examples you cannot otherwise deal with.

I'm also very curious, I fail to understand the need of not considering [0] as a FAM, esp. given that to my understanding, this is the only use of that syntax.

[kees]

In D126864#3646854, @jyknight wrote:

In D126864#3645994, @kees wrote:

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and sizeof([]) == error, they are being treated differently already by the compiler causing bugs in Linux. The kernel must still have a way to reject the _use_ of a [0] array. We cannot reject _declaration_ of them due to userspace API.

Looks like the linux kernel is currently chock-full of zero-length-arrays which are actually intended/required to work as flexible array members. Do you have a pending patch to convert all of them to [] which should be?

Well, yes and no. It's been a multi-year on-going effort. Though it's actually complete for the kernel now as of v5.18:
https://github.com/KSPP/linux/issues/78

What remains is the userspace headers. The _real_ pain has been the 1-element arrays. Ugh. The master bug is here:
https://github.com/KSPP/linux/issues/21

If you're saying you absolutely need this mode -- which I still believe would be nonsensical to provide -- I'd like to see the set of concrete examples you cannot otherwise deal with.

I have several goals. The most important is making sure all the fixed-sized trailing arrays are NOT treated as flexible arrays so that __builtin_object_size will work as expected for Linux's FORTIFY implementation and -fstrict-flex-arrays=2 certainly solves that. However, then we're left with all the 0-element arrays, which need to be cleaned up because they don't behave the same as true flexible arrays. For example, with sizeof; the kernel has dealt with allocation size bugs relating to sizeof being used against 0-element arrays (which _should_ have caused a build failure if it were a true flexible array and the bug would have been immediately found), and I would like to make sure those can never happen again.

If this were any other code base, I could just use -Wzero-length-array, but the scattered ancient userspace APIs need to live on until $forever.

And if this were any other code base, I could use #pragma, but it's frowned on in general, and is going to be met with even greater resistance in userspace API headers, especially given that it will need to be #ifdefed for clang vs gcc, and then tweaked again once GCC gains the option, which would then need to be version checked. (This is why #pragma is strongly discouraged: Linux has a whole system for doing compiler behavior detection for its internal headers and build system, but such things don't exist for the userspace headers.)

So given the behavioral differences that already existed between true flexible arrays and 0-element arrays, I'd wanted to have a way to turn off _all_ the flexible array "extensions". I never wanted these different levels for -fstrict-flex-arrays. :) I want the complete disabling of all the extensions that were creating inconsistent behaviors. To lose the ability to disable the 0-is-flexible would be disappointing, but if I absolutely have no choice, I can live with it. Given Linux will be the first user of this option, and the reason for this option being created, I'd _much_ rather have it doing what's needed, though. :)

[kees]

Example of the bug I want to block:

struct foo {
    int stuff;
    u32 data[0];
};

struct foo *deserialize(u8 *str, int len)
{
    struct foo *instance;
    size_t bytes;

    bytes = sizeof(*instance) + sizeof(instance->data) * (len / sizeof(u32));
    instance = kmalloc(bytes, GFP_KERNEL);
    if (!instance)
        return NULL;
    memcpy(instance->data, str, len)
}

This contains a catastrophic 1 character bug (should be sizeof(*instance->data)) that will only be encountered at runtime when the memcpy runs past the end of the the allocation. It could have been caught at build-time if the flex-array extensions were disabled; without -fstrict-flex-arrays=3 I have no way to block these (or similar) sneaking back into the kernel by way of old (or new) userspace APIs. :( So actually, even with #pragma, we could still trip over this. Please leave the =3 mode.

https://godbolt.org/z/dexd3a4Y8

[jyknight]

Just to move things forward here: I propose to continue and land this patch without mode 3 (there were only a couple comments left to address for that), and continue the discussion about whether to add mode 3 elsewhere. (I don't mind where, KSPP, gcc, or llvm issue trackers would all be fine by me.) Perhaps we add it later, perhaps we don't, but that doesn't need to hold up the rest.

[MaskRay]

Sound good to land the current form (we have sufficient tests for Sema and -fsanitize=array-bounds now) and have =3 as a separate discussion.

clang/test/CodeGen/bounds-checking-fam.c

8 ↗

(On Diff #440942)

84301 is the official CPython bug number. 94250 has been marked as a duplicate.

[serge-sans-paille]

Forgot the commit tag: just landed it as f764dc99b37e1e6428724a61f36bcb49c015dc70

[jyknight]

Sorry if I was unclear -- I did intend that the remaining minor open comments be addressed before commit. I've quoted them again here for clarity. Please address in a follow-up, thanks!

clang/include/clang/Driver/Options.td

1135

Minor whitespace issue.

clang/lib/CodeGen/CGExpr.cpp

906 ↗

(On Diff #442692)

Yes, we can reach this path, which is why the change is incorrect. We should not be changing the FAMness of undefined size, or size == 0, in any of the modes.

Please address.

clang/lib/Sema/SemaChecking.cpp

15972 ↗

(On Diff #443185)

What I meant to say is that both the IsTailPaddedMemberArray call, and this code, should be moved up above, in order to set IsUnboundedArray to true, so we will get the checks from that block.

(But doesn't need to be in this patch, fine to just leave a comment to that effect).

Please address (comment issue only).

15804 ↗

(On Diff #442592)

You didn't like the code change I suggested there?

Open question

15969 ↗

(On Diff #442592)

Is it redundant? This check is for an _incomplete_ type as the array element type, which doesn't seem directly related to unbounded array sizes. (though it also prevents knowing the size of the array)

Please address (comment issue only).

clang/test/CodeGen/object-size-flex-array.c

100

Please update all CHECK-LABELs to have open-paren, as per maskray's suggestion.

[chrish_ericsson_atx]

I've added a few suggestions for more rigorous testing, and raised a concern about a bug in this patch that is hiding expected -Warray-bounds warnings in certain cases.

clang/lib/CodeGen/CGExpr.cpp

888–889 ↗

(On Diff #443185)

I was exploring this difference and during my testing, I ran across what I think is a bug in mode 1 behavior of this patch:

https://godbolt.org/z/hj9T4MY61 -fsfa=0, no-warnings: f[0] f[ZERO] f[1] warnings: f[ONE] f[5]
https://godbolt.org/z/Ea9MY9jjh -fsfa=1, no-warnings: f[0] f[ZERO] f[5] warnings: f[1] f[ONE] <-- Incorrect behavior?
https://godbolt.org/z/aerMvxs6q -fsfa=2, no-warnings: f[0] f[ZERO] warnings: f[1] f[ONE] f[5]

I would think that -Warray-bounds should give a warning for accesses beyond the declared size of an array of 5 elements no matter what the -fstrict-flex-arrays=<n> mode is. Have I misunderstood?

Testing with compiler prior to this patch (using 14.0.0, e.g., and not giving -fsfa option) gives behavior consistent with -fsfa=0 on trunk. So it seems -Warray-bounds has always treated size>1 trailing arrays as concrete/fixed arrays, just as it does with non-trailing arrays. But I may be missing something, of course....

clang/lib/Sema/SemaChecking.cpp

15801–15803 ↗

(On Diff #443185)

Under what circumstances (what compiler options, what example code) would size>1 trailing arrays be treated as flexible array members? I'm not seeing that. -Warray-bounds warns about OOB access on size>1 trailing arrays, and always has (with the notable exception of the comment I made above, which I think is a bug).

clang/test/CodeGen/bounds-checking-fam.c

23 ↗

(On Diff #443185)

Should there be a "test_zero" case? Such as:

struct Zero {
   int a[0];
};

int test_zero(struct Zero *p, int i) {
   return p->a[i] + (p->a)[i];
}

Likewise, for C99 compilation, should there also be a case with a trailing incomplete array?

struct Inc {
   int x;
   int a[];
};

int test_inc(struct Inc *p, int i) {
   return p->a[i] + (p->a)[i];
}

clang/test/SemaCXX/array-bounds.cpp

211–213 ↗

(On Diff #443185)

This may be a bit beyond the scope of this change, but I think there are some problems with -Warray-bounds behavior that should be considered here as well, having to do with *nesting* of structs that have trailing size=1 arrays.

Consider https://godbolt.org/z/ex6P8bqY9, in which this code:

#define LEN 1

typedef struct S1 {
    int x;
    double y;
    int tail[3];
} S1;

typedef struct S {
    int   x;
    S1    s1;
    double y;
    int   tail[1];
} S;

void fooS(S *s) {
    s->tail[2] = 10;
    s->tail[5] = 20;
    (s->tail)[10] = 200;
    s->s1.tail[10] = (s->s1.tail)[10] + 1;
}

produces a warning (as you'd expect). But if you change the size of the trailing array in the nested struct to 1, you no longer get a warning: https://godbolt.org/z/dWET3E9d6

Note also that testing these examples with trunk build repeats the bug I mentioned in an earlier comment, where -fsfa=1 causes an expected -Warray-bounds warning to be dropped.

[sberg]

I'm surprised that

$ cat test.c
struct S {
    int m1;
    int m2[1];
};
void f(struct S * s) {
    s->m2[1] = 0;
}

$ clang -fsyntax-only -fstrict-flex-arrays=1 test.c
test.c:6:5: warning: array index 1 is past the end of the array (which contains 1 element) [-Warray-bounds]
    s->m2[1] = 0;
    ^     ~
test.c:3:5: note: array 'm2' declared here
    int m2[1];
    ^
1 warning generated.

causes a warning? I would have expected it to be suppressed in this case, as with the lax -fstrict-flex-arrays=0 default, and only to hit with the stricter -fstrict-flex-arrays=2.

[sberg]

In D126864#3698798, @sberg wrote:

I'm surprised that

[...]

causes a warning? I would have expected it to be suppressed in this case, as with the lax -fstrict-flex-arrays=0 default, and only to hit with the stricter -fstrict-flex-arrays=2.

appears to be addressed with https://reviews.llvm.org/D132853 "[clang] Fix -Warray-bound interaction with -fstrict-flex-arrays=1", thanks

[aaron.ballman]

clang/docs/ClangCommandLineReference.rst

2624

From the top of this file:

-------------------------------------------------------------------
NOTE: This file is automatically generated by running clang-tblgen
-gen-opt-docs. Do not edit this file by hand!!
-------------------------------------------------------------------

so none of these changes are reflected at: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fstrict-flex-arrays

This documentation needs to live alongside the option.

2626

This phrasing is problematic because it uses the term "flexible array members" which is a term of art in the standard when it's really talking about something else. We don't want int trailing[1]; to be considered a flexible array member in terms of the language rules.

[msebor]

clang/docs/ClangCommandLineReference.rst

2626

A pedantic reading of the text would suggest that the type of s.trailing is incomplete at level 1 and sizeof s.trailing is invalid (requiring an error). Another minor issue is that the text refers to a size of the array when it actually means the number of elements.

[steakhal]

I noticed some inconsistency between this -fstrict-flex-arrays=N flag and what the RecordDecl::hasFlexibleArrayMember() returns for an example like this:

typedef unsigned long size_t;
void *malloc(size_t);
void free(void *);

void field(void) {
  struct vec { size_t len; int data[0]; };
  struct vec *a = (struct vec*) malloc(sizeof(struct vec) + 10*sizeof(int));
  free(a);
}

In the example, I use don't specify the -fstrict-flex-arrays flag, thus it should default to 0, which means that any trailing arrays (let it be incomplete or of any concrete size), to be considered as a flexible-array-member.
In the AST, for the RecordDecl, I'd expect that the hasFlexibleArrayMember() member function would reflect this, and return true, however, it returns false instead.
It does so because in SemaDecl.cpp Sema::ActOnFields() before doing some FAM-related checks and diagnostics it performs this check, guarding that branch:

bool IsLastField = (i + 1 == Fields.end());
if (FDTy->isFunctionType()) {
  // ...
} else if (FDTy->isIncompleteArrayType() &&
           (Record || isa<ObjCContainerDecl>(EnclosingDecl))) {
  //This is the FAM handling branch.
  // ...
}

Consequently, Sema will only set the bit for hasFlexibleArrayMember if the array is incomplete.
So my question is, if the RecordDecl::hasFlexibleArrayMember() should be consistent with the -fstrict-flex-arrays flag, or not?
@serge-sans-paille @aaron.ballman

[efriedma]

RecordDecl::hasFlexibleArrayMember() is supposed to reflect the standard's definition of a flexible array member, which only includes incomplete arrays. The places that care about other array members use separate checks. We wouldn't want to accidentally extend the non-standard treatment of arrays with fixed bound to other places.

I think there was an effort to try to refactor uses of StrictFlexArrays to a centralized helper, but I don't remember what happened to it.

[aaron.ballman]

In D126864#4586307, @efriedma wrote:

RecordDecl::hasFlexibleArrayMember() is supposed to reflect the standard's definition of a flexible array member, which only includes incomplete arrays. The places that care about other array members use separate checks. We wouldn't want to accidentally extend the non-standard treatment of arrays with fixed bound to other places.

I think there was an effort to try to refactor uses of StrictFlexArrays to a centralized helper, but I don't remember what happened to it.

Are you thinking of https://reviews.llvm.org/D134791 perchance?

[efriedma]

I was probably thinking of that, yes.

[steakhal]

Thanks for the xrefs! It turns out I've already seen that one :D
I'll look into it once more, with a fresh set of eyes. Thanks again!

clang/docs/ClangCommandLineReference.rst

 Enable unstable and experimental features
 
 .. option:: -funwind-tables, -fno-unwind-tables
 
 .. option:: -fuse-cxa-atexit, -fno-use-cxa-atexit
 
 .. option:: -fuse-init-array, -fno-use-init-array
 
+.. option:: -fstrict-flex-arrays
+
+Do not consider sized arrays at the end of a struct as flexible arrays.
+
 .. option:: -fuse-ld=<arg>
 
 .. option:: -fuse-line-directives, -fno-use-line-directives
 
 Use #line in preprocessed output
 
 .. option:: -fvalidate-ast-input-files-content
 

clang/docs/ReleaseNotes.rst

- Clang now supports randomizing structure layout in C. This feature is a
   A seed value is required to enable randomization, and is deterministic based
   on a seed value. Use the ``-frandomize-layout-seed=`` or
   ``-frandomize-layout-seed-file=`` flags.
 
   .. note::
 
       Randomizing structure layout is a C-only feature.
 
+- Clang now supports the ``-fstrict-flex-arrays`` to only consider trailing
+  arrays with unknown size arrays as flexible arrays. This breaks compatibility
+  with some legacy code but allows for better folding of
+  ``__builtin_object_size``.
+
 Bug Fixes
 ---------
 - ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
   wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array
   size expression. This was fixed and ``::getArraySize()`` will now always
   either return ``None`` or a ``llvm::Optional`` wrapping a valid ``Expr*``.
   This fixes `Issue 53742 <https://github.com/llvm/llvm-project/issues/53742>`_.
 - We now ignore full expressions when traversing cast subexpressions. This

clang/include/clang/Basic/LangOptions.def

 
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
 LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
         "unsigned fixed point types having one extra padding bit")
 
 LANGOPT(RegisterStaticDestructors, 1, 1, "Register C++ static destructors")
 
 LANGOPT(MatrixTypes, 1, 0, "Enable or disable the builtin matrix type")
+LANGOPT(StrictFlexArrays, 1, 0, "Rely on strict definition of flexible arrays")
 
 COMPATIBLE_VALUE_LANGOPT(MaxTokens, 32, 0, "Max number of tokens per TU or 0")
 
 ENUM_LANGOPT(SignReturnAddressScope, SignReturnAddressScopeKind, 2, SignReturnAddressScopeKind::None,
              "Scope of return address signing")
 ENUM_LANGOPT(SignReturnAddressKey, SignReturnAddressKeyKind, 1, SignReturnAddressKeyKind::AKey,
              "Key used for return address signing")
 LANGOPT(BranchTargetEnforcement, 1, 0, "Branch-target enforcement enabled")

clang/include/clang/Driver/Options.td

 defm allow_editor_placeholders : BoolFOption<"allow-editor-placeholders",
   LangOpts<"AllowEditorPlaceholders">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Treat editor placeholders as valid source code">,
   NegFlag<SetFalse>>;
 def fallow_unsupported : Flag<["-"], "fallow-unsupported">, Group<f_Group>;
 def fapple_kext : Flag<["-"], "fapple-kext">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use Apple's kernel extensions ABI">,
   MarshallingInfoFlag<LangOpts<"AppleKext">>;
+defm strict_flex_arrays : BoolFOption<"strict-flex-arrays",
+  LangOpts<"StrictFlexArrays">, DefaultFalse,
+  PosFlag<SetTrue, [CC1Option], "Enable optimizations based on the strict definition of flexible arrays">,
+  NegFlag<SetFalse>>;
 defm apple_pragma_pack : BoolFOption<"apple-pragma-pack",
   LangOpts<"ApplePragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable Apple gcc-compatible #pragma pack handling">,
   NegFlag<SetFalse>>;
 defm xl_pragma_pack : BoolFOption<"xl-pragma-pack",
   LangOpts<"XLPragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable IBM XL #pragma pack handling">,
   NegFlag<SetFalse>>;

clang/lib/AST/ExprConstant.cpp

   //   otherwise seemingly fine. It may be useful to reintroduce this behavior
   //   with some sort of list. OTOH, it seems that GCC is always
   //   conservative with the last element in structs (if it's an array), so our
   //   current behavior is more compatible than an explicit list approach would
   //   be.
   return LVal.InvalidBase &&
          Designator.Entries.size() == Designator.MostDerivedPathLength &&
          Designator.MostDerivedIsArrayElement &&
+         (Designator.isMostDerivedAnUnsizedArray() ||
+          !Ctx.getLangOpts().StrictFlexArrays) &&
          isDesignatorAtObjectEnd(Ctx, LVal);
 }
 
 /// Converts the given APInt to CharUnits, assuming the APInt is unsigned.
 /// Fails if the conversion would cause loss of precision.
 static bool convertUnsignedAPIntToCharUnits(const llvm::APInt &Int,
                                             CharUnits &Result) {
   auto CharUnitsMax = std::numeric_limits<CharUnits::QuantityType>::max();

clang/lib/Driver/ToolChains/Clang.cpp

 
   Args.AddLastArg(CmdArgs, options::OPT_ffinite_loops,
                   options::OPT_fno_finite_loops);
 
   Args.AddLastArg(CmdArgs, options::OPT_fwritable_strings);
   Args.AddLastArg(CmdArgs, options::OPT_funroll_loops,
                   options::OPT_fno_unroll_loops);
 
+  Args.addOptInFlag(CmdArgs, options::OPT_fstrict_flex_arrays,
+                    options::OPT_fno_strict_flex_arrays);
+
   Args.AddLastArg(CmdArgs, options::OPT_pthread);
 
   if (Args.hasFlag(options::OPT_mspeculative_load_hardening,
                    options::OPT_mno_speculative_load_hardening, false))
     CmdArgs.push_back(Args.MakeArgString("-mspeculative-load-hardening"));
 
   RenderSSPOptions(D, TC, Args, CmdArgs, KernelOrKext);
   RenderSCPOptions(TC, Args, CmdArgs);

clang/test/CodeGen/object-size-flex-array.c

+// RUN: %clang -fstrict-flex-arrays    -target x86_64-apple-darwin -S -emit-llvm %s -o - 2>&1 | FileCheck --check-prefix CHECK-STRICT %s
+// RUN: %clang -fno-strict-flex-arrays -target x86_64-apple-darwin -S -emit-llvm %s -o - 2>&1 | FileCheck %s
+
+#ifndef DYNAMIC
+#define OBJECT_SIZE_BUILTIN __builtin_object_size
+#else
+#define OBJECT_SIZE_BUILTIN __builtin_dynamic_object_size
+#endif
+
+typedef struct {
+  float f;
+  double c[];
+} foo_t;
+
+typedef struct {
+  float f;
+  double c[0];
+} foo0_t;
+
+typedef struct {
+  float f;
+  double c[1];
+} foo1_t;
+
+typedef struct {
+  float f;
+  double c[2];
+} foo2_t;
+
+// CHECK-LABEL: @bar
+// CHECK-STRICT-LABEL: @bar
+unsigned bar(foo_t *f) {
+  // CHECK: ret i32 %
+  // CHECK-STRICT: ret i32 %
+  return OBJECT_SIZE_BUILTIN(f->c, 1);
+}
+
+// CHECK-LABEL: @bar0
+// CHECK-STRICT-LABEL: @bar
+unsigned bar0(foo0_t *f) {
+  // CHECK: ret i32 %
+  // CHECK-STRICT: ret i32 0
+  return OBJECT_SIZE_BUILTIN(f->c, 1);
+}
+
+// CHECK-LABEL: @bar1
+// CHECK-STRICT-LABEL: @bar
+unsigned bar1(foo1_t *f) {
+  // CHECK: ret i32 %
+  // CHECK-STRICT: ret i32 8
+  return OBJECT_SIZE_BUILTIN(f->c, 1);
+}
+
+// CHECK-LABEL: @bar2
+// CHECK-STRICT-LABEL: @bar
+unsigned bar2(foo2_t *f) {
+  // CHECK: ret i32 %
+  // CHECK-STRICT: ret i32 16
+  return OBJECT_SIZE_BUILTIN(f->c, 1);
+}