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

Mon Jul 18 09:20:25 PDT 2022

jyknight added a comment.

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!

================
Comment at: clang/include/clang/Driver/Options.td:1143
   MarshallingInfoFlag<LangOpts<"AppleKext">>;
+def fstrict_flex_arrays_EQ : Joined<["-"], "fstrict-flex-arrays=">,Group<f_Group>,
+  MetaVarName<"<n>">, Values<"0,1,2,3">,
----------------
MaskRay wrote:
> 
Minor whitespace issue.

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:906
       // member, only a T[0] or T[] member gets that treatment.
+      // Under StrictFlexArraysLevel, obey c99+ that disallows FAM in union, see
+      // C11 6.7.2.1 §18
----------------
jyknight wrote:
> kees wrote:
> > jyknight wrote:
> > > serge-sans-paille wrote:
> > > > jyknight wrote:
> > > > > 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.)
> > > > 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.
> > > 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.)
> > > `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.. */
> >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ```
> > 
> 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.
> 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.

================
Comment at: clang/lib/Sema/SemaChecking.cpp:15972

+    // FIXME: this check should belong to the IsTailPaddedMemberArray call
+    // below.
----------------
jyknight wrote:
> 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).
> 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).

================
Comment at: clang/lib/Sema/SemaChecking.cpp:15804
+
+  // FIXME: we should also allow Size = 0 here per the definition of
+  // StrictFlexArraysLevel, but that's backward incompatible with previous clang
----------------
jyknight wrote:
> jyknight wrote:
> > serge-sans-paille wrote:
> > > jyknight wrote:
> > > > 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?
> > > correct
> > 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;`
> > ```
> > 
> You didn't like the code change I suggested there?
> You didn't like the code change I suggested there?

Open question

================
Comment at: clang/lib/Sema/SemaChecking.cpp:15969
     // access which precedes the array bounds.
     if (BaseType->isIncompleteType())
       return;
----------------
jyknight wrote:
> jyknight wrote:
> > serge-sans-paille wrote:
> > > And here
> > 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...
> 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)
> 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).

================
Comment at: clang/test/CodeGen/object-size-flex-array.c:99
+
+// CHECK-LABEL: @babar2
+unsigned babar2(foofoo2_t *f) {
----------------
MaskRay wrote:
> 
Please update all CHECK-LABELs to have open-paren, as per maskray's suggestion.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126864/new/

https://reviews.llvm.org/D126864