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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
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
----------------
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.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15972
 
+    // FIXME: this check should belong to the IsTailPaddedMemberArray call
+    // below.
----------------
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).


================
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:
> 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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864