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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I've added a few suggestions for more rigorous testing, and raised a concern about a bug in this patch that is hiding expected -Warray-bounds warnings in certain cases.



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:888-889
   if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
     // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
     // was produced by macro expansion.
+    if (StrictFlexArraysLevel >= 2 && CAT->getSize().ugt(0))
----------------
I was exploring this difference and during my testing, I ran across what I think is a bug in mode 1 behavior of this patch:

https://godbolt.org/z/hj9T4MY61 -fsfa=0, no-warnings: f[0] f[ZERO] f[1] warnings: f[ONE] f[5]
https://godbolt.org/z/Ea9MY9jjh -fsfa=1, no-warnings: f[0] f[ZERO] f[5] warnings: f[1] f[ONE]  <-- Incorrect behavior?
https://godbolt.org/z/aerMvxs6q -fsfa=2, no-warnings: f[0] f[ZERO] warnings: f[1] f[ONE] f[5]

I would think that -Warray-bounds should give a warning for accesses beyond the declared size of an array of 5 elements no matter what the -fstrict-flex-arrays=<n> mode is.  Have I misunderstood?

Testing with compiler prior to this patch (using 14.0.0, e.g., and not giving -fsfa option) gives behavior consistent with -fsfa=0 on trunk.  So it seems -Warray-bounds has always treated size>1 trailing arrays as concrete/fixed arrays, just as it does with non-trailing arrays.  But I may be missing something, of course....


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15801-15803
+  // 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...
----------------
Under what circumstances (what compiler options, what example code) would size>1 trailing arrays be treated as flexible array members?  I'm not seeing that.  -Warray-bounds warns about OOB access on size>1 trailing arrays, and always has (with the notable exception of the comment I made above, which I think is a bug).


================
Comment at: clang/test/CodeGen/bounds-checking-fam.c:23
 // CHECK-LABEL: define {{.*}} @{{.*}}test_one{{.*}}(
 int test_one(struct One *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan
----------------
Should there be a "test_zero" case?  Such as:

```
struct Zero {
   int a[0];
};

int test_zero(struct Zero *p, int i) {
   return p->a[i] + (p->a)[i];
}
```

Likewise, for C99 compilation, should there also be a case with a trailing incomplete array?

```
struct Inc {
   int x;
   int a[];
};

int test_inc(struct Inc *p, int i) {
   return p->a[i] + (p->a)[i];
}
```



================
Comment at: clang/test/SemaCXX/array-bounds.cpp:211-213
+struct foo {
+  char c[ONE]; // expected-note {{array 'c' declared here}}
+};
----------------
This may be a bit beyond the scope of this change, but I think there are some problems with -Warray-bounds behavior that should be considered here as well, having to do with *nesting* of structs that have trailing size=1 arrays.

Consider https://godbolt.org/z/ex6P8bqY9, in which this code:

```
#define LEN 1

typedef struct S1 {
    int x;
    double y;
    int tail[3];
} S1;

typedef struct S {
    int   x;
    S1    s1;
    double y;
    int   tail[1];
} S;

void fooS(S *s) {
    s->tail[2] = 10;
    s->tail[5] = 20;
    (s->tail)[10] = 200;
    s->s1.tail[10] = (s->s1.tail)[10] + 1;
}
```
produces a warning (as you'd expect).  But if you change the size of the trailing array in the **nested** struct to 1, you no longer get a warning: https://godbolt.org/z/dWET3E9d6

Note also that testing these examples with trunk build repeats the bug I mentioned in an earlier comment, where -fsfa=1 causes an expected -Warray-bounds warning to be dropped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864