[clang] cleanup -fstrict-flex-arrays implementation

[serge-sans-paille]

Summary

This is a follow up to https://reviews.llvm.org/D126864, addressing some remaining
comments.

It also considers union with a single zero-length array field as FAM for each
value of -fstrict-flex-arrays.

[jyknight]

clang/lib/CodeGen/CGExpr.cpp

910

This is a functional change (which is good, but the commit message needs to be adjusted). In current trunk

union X { int x[0]; };
int foo(X*x) { return x->x[2]; }

built with -fstrict-flex-arrays=2 -fsanitize=array-bounds would incorrectly report ubsan error, and this change fixes that.

I think this testcase can be added to clang/test/CodeGen/bounds-checking-fam.c. Probably this should also be nominated for backport to the 15.x branch (for the first point release I expect)

[jyknight]

This comment was removed by jyknight.

[jyknight]

(please ignore the last comment, I sent it to the wrong review thread)

clang/include/clang/Driver/Options.td

 defm allow_editor_placeholders : BoolFOption<"allow-editor-placeholders",
   LangOpts<"AllowEditorPlaceholders">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Treat editor placeholders as valid source code">,
   NegFlag<SetFalse>>;
 def fallow_unsupported : Flag<["-"], "fallow-unsupported">, Group<f_Group>;
 def fapple_kext : Flag<["-"], "fapple-kext">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use Apple's kernel extensions ABI">,
   MarshallingInfoFlag<LangOpts<"AppleKext">>;
-def fstrict_flex_arrays_EQ : Joined<["-"], "fstrict-flex-arrays=">,Group<f_Group>,
+def fstrict_flex_arrays_EQ : Joined<["-"], "fstrict-flex-arrays=">, Group<f_Group>,
   MetaVarName<"<n>">, Values<"0,1,2">,
   LangOpts<"StrictFlexArrays">,
   Flags<[CC1Option]>,
   HelpText<"Enable optimizations based on the strict definition of flexible arrays">,
   MarshallingInfoInt<LangOpts<"StrictFlexArrays">>;
 defm apple_pragma_pack : BoolFOption<"apple-pragma-pack",
   LangOpts<"ApplePragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable Apple gcc-compatible #pragma pack handling">,

clang/lib/CodeGen/CGExpr.cpp

static bool isFlexibleArrayMemberExpr(const Expr *E,
 
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast<MemberExpr>(E)) {
     // FIXME: If the base type of the member expr is not FD->getParent(),
     // this should not be treated as a flexible array member access.
     if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
       // FIXME: Sema doesn't treat a T[1] union member as a flexible array
       // 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
       if (FD->getParent()->isUnion())
-        return StrictFlexArraysLevel < 2;
+        return true;
       RecordDecl::field_iterator FI(
           DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
       return ++FI == FD->getParent()->field_end();
     }
   } else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E)) {
     return IRE->getDecl()->getNextIvar() == nullptr;
   }
 

clang/lib/Sema/SemaChecking.cpp

 /// We avoid emitting out-of-bounds access warnings for such arrays as they are
 /// commonly used to emulate flexible arrays in C89 code.
 static bool IsTailPaddedMemberArray(Sema &S, const llvm::APInt &Size,
                                     const NamedDecl *ND,
                                     unsigned StrictFlexArraysLevel) {
   if (!ND)
     return false;
 
-  if (StrictFlexArraysLevel >= 2 && Size != 0)
-    return false;
-
-  if (StrictFlexArraysLevel == 1 && Size.uge(2))
-    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 == 0 && Size != 1)
+  if (StrictFlexArraysLevel >= 2 || Size.uge(2))
     return false;
 
   const FieldDecl *FD = dyn_cast<FieldDecl>(ND);
   if (!FD)
     return false;
 
   // Don't consider sizes resulting from macro expansions or template argument
   // substitution to form C89 tail-padded arrays.
if (index.isUnsigned() || !index.isNegative()) {
     // access exceeds the array bounds. However we can still diagnose an array
     // access which precedes the array bounds.
     //
     // FIXME: this check should be redundant with the IsUnboundedArray check
     // above.
     if (BaseType->isIncompleteType())
       return;
 
-    // FIXME: this check should belong to the IsTailPaddedMemberArray call
-    // below.
+    // FIXME: this check should be used to set IsUnboundedArray from the
+    // beginning.
     llvm::APInt size = ArrayTy->getSize();
     if (!size.isStrictlyPositive())
       return;
 
     if (BaseType != EffectiveType) {
       // Make sure we're comparing apples to apples when comparing index to size
       uint64_t ptrarith_typesize = Context.getTypeSize(EffectiveType);
       uint64_t array_typesize = Context.getTypeSize(BaseType);

clang/test/CodeGen/bounds-checking-fam.c

 // CHECK-LABEL: define {{.*}} @{{.*}}test_two{{.*}}(
 int test_two(struct Two *p, int i) {
   // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 
+union uZero {
+  int a[0];
+};
+union uOne {
+  int a[1];
+};
+union uTwo {
+  int a[2];
+};
+union uThree {
+  int a[3];
+};
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_uzero{{.*}}(
+int test_uzero(union uZero *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1-NOT: @__ubsan
+  // CHECK-STRICT-2-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_uone{{.*}}(
+int test_uone(union uOne *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1-NOT: @__ubsan
+  // CHECK-STRICT-2: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_utwo{{.*}}(
+int test_utwo(union uTwo *p, int i) {
+  // CHECK-STRICT-0: @__ubsan
+  // CHECK-STRICT-1: @__ubsan
+  // CHECK-STRICT-2: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_uthree{{.*}}(
+int test_uthree(union uThree *p, int i) {
+  // CHECK-STRICT-0: @__ubsan
+  // CHECK-STRICT-1: @__ubsan
+  // CHECK-STRICT-2: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
 // CHECK-LABEL: define {{.*}} @{{.*}}test_three{{.*}}(
 int test_three(struct Three *p, int i) {
   // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 

clang/test/CodeGen/object-size-flex-array.c

typedef struct {
   double c[1];
 } foo1_t;
 
 typedef struct {
   float f;
   double c[2];
 } foo2_t;
 
-// CHECK-LABEL: @bar
+// CHECK-LABEL: @bar(
 unsigned bar(foo_t *f) {
   // CHECK-STRICT-0: ret i32 %
   // CHECK-STRICT-1: ret i32 %
   // CHECK-STRICT-2: ret i32 %
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
-// CHECK-LABEL: @bar0
+// CHECK-LABEL: @bar0(
 unsigned bar0(foo0_t *f) {
   // CHECK-STRICT-0: ret i32 %
   // CHECK-STRICT-1: ret i32 %
   // CHECK-STRICT-2: ret i32 %
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
-// CHECK-LABEL: @bar1
+// CHECK-LABEL: @bar1(
 unsigned bar1(foo1_t *f) {
   // CHECK-STRICT-0: ret i32 %
   // CHECK-STRICT-1: ret i32 %
   // CHECK-STRICT-2: ret i32 8
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
-// CHECK-LABEL: @bar2
+// CHECK-LABEL: @bar2(
 unsigned bar2(foo2_t *f) {
   // CHECK-STRICT-0: ret i32 %
   // CHECK-STRICT-1: ret i32 16
   // CHECK-STRICT-2: ret i32 16
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
 // Also checks for non-trailing flex-array like members
 
 typedef struct {
   double c[0];
   float f;
 } foofoo0_t;
 
 typedef struct {
   double c[1];
   float f;
 } foofoo1_t;
 
 typedef struct {
   double c[2];
   float f;
 } foofoo2_t;
 
-// CHECK-LABEL: @babar0
+// CHECK-LABEL: @babar0(
 unsigned babar0(foofoo0_t *f) {
   // CHECK-STRICT-0: ret i32 0
   // CHECK-STRICT-1: ret i32 0
   // CHECK-STRICT-2: ret i32 0
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
-// CHECK-LABEL: @babar1
+// CHECK-LABEL: @babar1(
 unsigned babar1(foofoo1_t *f) {
   // CHECK-STRICT-0: ret i32 8
   // CHECK-STRICT-1: ret i32 8
   // CHECK-STRICT-2: ret i32 8
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }
 
-// CHECK-LABEL: @babar2
+// CHECK-LABEL: @babar2(
 unsigned babar2(foofoo2_t *f) {
   // CHECK-STRICT-0: ret i32 16
   // CHECK-STRICT-1: ret i32 16
   // CHECK-STRICT-2: ret i32 16
   return OBJECT_SIZE_BUILTIN(f->c, 1);
 }