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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
