[test] Check for more -fsanitize=array-bounds regressions

[sberg]

Summary

...that had been introduced with (since reverted) https://github.com/llvm/llvm-project/commit/886715af962de2c92fac4bd37104450345711e4a "[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays", and caused issues in the wild:

For one, the HarfBuzz project has various "fake" flexible array members of the form

Type                arrayZ[HB_VAR_ARRAY];

in https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-open-type.hh, where HB_VAR_ARRAY is a macro defined as

#ifndef HB_VAR_ARRAY
#define HB_VAR_ARRAY 1
#endif

in https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-machinery.hh.

For another, the Firebird project in https://github.com/FirebirdSQL/firebird/blob/master/src/lock/lock_proto.h uses a trailing member

srq lhb_hash[1];                        // Hash table

as a "fake" flexible array, but declared in a

struct lhb : public Firebird::MemoryHeader

that is not a standard-layout class (because the Firebird::MemoryHeader base class also declares non-static data members).

(Checking for the second case required changing the test file from C to C++.)

[MaskRay]

Do we need a C test (just add a -x c RUN line)? @serge-sans-paille Do you think we may likely make C++ stricter than C?

[serge-sans-paille]

In D128783#3617924, @MaskRay wrote:

Do we need a C test (just add a -x c RUN line)? @serge-sans-paille Do you think we may likely make C++ stricter than C?

I've started a similar discussion on https://reviews.llvm.org/D126864, but I'm fine to discuss that here :-)

Some extra data points:

GCC doesn't special case macro expansion, nor non-standard layout, not even size that result from the expansion of a template parameter.

I *guess* bounds resulting from macro expansion could result from preprocessor parameters and then are somehow variable.

I *guess* bounds resulting from template parameter expansion could result from various template instantiation and then are somehow variable.

I don't have a clue about non-standard layout interaction with FAM.

Following Chesterton's fence we should understand why this was introduced in Clang, because it turns out this actually causes issues to (some) users, and without that knowledge I'd be in favor of adopting GCC behavior and not making C++ stricter than C here.

[serge-sans-paille]

And note that with current clang, the behavior I describe here is not uniform across clang code base :-/

[sberg]

Updated the prospective git commit message as follow:

[test] Check for more -fsanitize=array-bounds behavior

...that had temporarily regressed with (since reverted)
<https://github.com/llvm/llvm-project/commit/886715af962de2c92fac4bd37104450345711e4a>
"[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible
arrays", and had then been seen to cause issues in the wild:

For one, the HarfBuzz project has various "fake" flexible array members of the
form

> Type                arrayZ[HB_VAR_ARRAY];

in <https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-open-type.hh>, where
HB_VAR_ARRAY is a macro defined as

> #ifndef HB_VAR_ARRAY
> #define HB_VAR_ARRAY 1
> #endif

in <https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-machinery.hh>.

For another, the Firebird project in
<https://github.com/FirebirdSQL/firebird/blob/master/src/lock/lock_proto.h> uses
a trailing member

>         srq lhb_hash[1];                        // Hash table

as a "fake" flexible array, but declared in a

> struct lhb : public Firebird::MemoryHeader

that is not a standard-layout class (because the Firebird::MemoryHeader base
class also declares non-static data members).

(The second case is specific to C++.  Extend the test setup so that all the
other tests are now run for both C and C++, just in case the behavior could ever
start to diverge for those two languages.)

Differential Revision: https://reviews.llvm.org/D128783

[serge-sans-paille]

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

55

Another C++-specific behavior: if the bound results from the substitution of a template parameter, event if its value is 1 it's currently not considered a FAM

[sberg]

added test involving template argument substitution

[sberg]

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

55

I hadn't encountered that in the wild, so didn't bother to create a test for it; but yes, makes sense to add it too

[MaskRay]

clang/test/SemaCXX/array-bounds.cpp has tailpad and metaprogramming tests which may be added here.

Happy when @serge-sans-paille is happy.

[serge-sans-paille]

I'm fine with these tests has they reflect current implementation. But beware that ubsan and -Warray-bounds don't behave the same wrt. FAM, which I find disturbing. I'll discuss that in another review.

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

 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fsanitize=array-bounds %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-0
+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fsanitize=array-bounds -x c++ %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-0,CXX,CXX-STRICT-0
 
 /// Before flexible array member was added to C99, many projects use a
 /// one-element array as the last emember of a structure as an alternative.
 /// E.g. https://github.com/python/cpython/issues/84301
 /// Suppress such errors by default.
 struct One {
   int a[1];
 };
 struct Two {
   int a[2];
 };
 struct Three {
   int a[3];
 };
 
-// CHECK-LABEL: define {{.*}} @test_one(
+// CHECK-LABEL: define {{.*}} @{{.*}}test_one{{.*}}(
 int test_one(struct One *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan
   return p->a[i] + (p->a)[i];
 }
 
-// CHECK-LABEL: define {{.*}} @test_two(
+// CHECK-LABEL: define {{.*}} @{{.*}}test_two{{.*}}(
 int test_two(struct Two *p, int i) {
   // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 
-// CHECK-LABEL: define {{.*}} @test_three(
+// CHECK-LABEL: define {{.*}} @{{.*}}test_three{{.*}}(
 int test_three(struct Three *p, int i) {
   // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
+
+#define FLEXIBLE 1
+struct Macro {
+  int a[FLEXIBLE];
+};
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_macro{{.*}}(
+int test_macro(struct Macro *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+#if defined __cplusplus
+
+struct Base {
+  int b;
+};
+struct NoStandardLayout : Base {
+  int a[1];
+};
+
+// CXX-LABEL: define {{.*}} @{{.*}}test_nostandardlayout{{.*}}(
+int test_nostandardlayout(NoStandardLayout *p, int i) {
+  // CXX-STRICT-0-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+template<int N> struct Template {
+  int a[N];
+};
+
+// CXX-LABEL: define {{.*}} @{{.*}}test_template{{.*}}(
+int test_template(Template<1> *p, int i) {
+  // CXX-STRICT-0-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+#endif