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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D126864#3645994 <https://reviews.llvm.org/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.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864