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