[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:892
+      return false;
     if (CAT->getSize().ugt(1))
       return false;
----------------
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...



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


================
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
----------------
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;`
```



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


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
 
+      if (getContext().getLangOpts().StrictFlexArrays >= 2)
+        return false;
----------------
Yuk, another place that is weird and doesn't handle StrictFlexArrays as expected...


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

https://reviews.llvm.org/D126864