| Message ID | 20240319151032.732309-2-stefansf@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | analyzer: Bail out on function pointer for -Wanalyzer-allocation-size | expand |
On Tue, 2024-03-19 at 16:10 +0100, Stefan Schulze Frielinghaus wrote: > On s390 pr94688.c is failing due to excess error > > pr94688.c:6:5: warning: allocated buffer size is not a multiple of > the pointee's size [CWE-131] [-Wanalyzer-allocation-size] > > This is because on s390 functions are by default aligned to an 8-byte > boundary and during function type construction size is set to > function > boundary. Thus, for the assignment > > a.0_1 = (void (*<T237>) ()) &a; > > we have that the right-hand side is pointing to a 4-byte memory > region > whereas the size of the function pointer is 8 byte and a warning is > emitted. FWIW the test case in question is a regression test for an ICE seen in the GCC 10 implementation of the analyzer, which was fixed by the big rewrite in r11-2694-g808f4dfeb3a95f. So the code in the test doesn't make a great deal of sense. > > I could follow and skip this test as done in PR112705, or we could > bail > out early in the analyzer for function pointers. My intuition so far > is that -Wanalyzer-allocation-size shouldn't care about function > pointer. Therefore, I went for bailing out early. If you believe > this > is wrong I can still go by skipping this test on s390. Any thoughts? I tried imagining a situation where we're analyzing a function generated at run-time, but it strikes me that the buffer allocated for such a function can be of arbitrary size. So -Wanalyzer-allocation- size is meaningless for functions. There's probably a case for checking for mismatches between pointers to code vs pointers to data (e.g. alignments, Harvard architecture machines, etc), but -Wanalyzer-allocation-size doesn't do that. So I think your patch is correct. OK to push it if it passes bootstrap®ression testing. Thanks Dave > --- > gcc/analyzer/region-model.cc | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > model.cc > index f079d1fb37e..1b43443d168 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -3514,6 +3514,10 @@ region_model::check_region_size (const region > *lhs_reg, const svalue *rhs_sval, > || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE) > return; > > + /* Bail out early on function pointers. */ > + if (TREE_CODE (pointee_type) == FUNCTION_TYPE) > + return; > + > /* Bail out early on pointers to structs where we can > not deduce whether the buffer size is compatible. */ > bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type);
On Tue, Mar 19, 2024 at 12:38:34PM -0400, David Malcolm wrote: > On Tue, 2024-03-19 at 16:10 +0100, Stefan Schulze Frielinghaus wrote: > > On s390 pr94688.c is failing due to excess error > > > > pr94688.c:6:5: warning: allocated buffer size is not a multiple of > > the pointee's size [CWE-131] [-Wanalyzer-allocation-size] > > > > This is because on s390 functions are by default aligned to an 8-byte > > boundary and during function type construction size is set to > > function > > boundary. Thus, for the assignment > > > > a.0_1 = (void (*<T237>) ()) &a; > > > > we have that the right-hand side is pointing to a 4-byte memory > > region > > whereas the size of the function pointer is 8 byte and a warning is > > emitted. > > FWIW the test case in question is a regression test for an ICE seen in > the GCC 10 implementation of the analyzer, which was fixed by the big > rewrite in r11-2694-g808f4dfeb3a95f. > > So the code in the test doesn't make a great deal of sense. > > > > > I could follow and skip this test as done in PR112705, or we could > > bail > > out early in the analyzer for function pointers. My intuition so far > > is that -Wanalyzer-allocation-size shouldn't care about function > > pointer. Therefore, I went for bailing out early. If you believe > > this > > is wrong I can still go by skipping this test on s390. Any thoughts? > > I tried imagining a situation where we're analyzing a function > generated at run-time, but it strikes me that the buffer allocated for > such a function can be of arbitrary size. So -Wanalyzer-allocation- > size is meaningless for functions. > > There's probably a case for checking for mismatches between pointers to > code vs pointers to data (e.g. alignments, Harvard architecture > machines, etc), but -Wanalyzer-allocation-size doesn't do that. > > So I think your patch is correct. > > OK to push it if it passes bootstrap®ression testing. Bootstrapped and regtested on x64 and s390x. Thanks, Stefan > > Thanks > Dave > > > --- > > gcc/analyzer/region-model.cc | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > > model.cc > > index f079d1fb37e..1b43443d168 100644 > > --- a/gcc/analyzer/region-model.cc > > +++ b/gcc/analyzer/region-model.cc > > @@ -3514,6 +3514,10 @@ region_model::check_region_size (const region > > *lhs_reg, const svalue *rhs_sval, > > || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE) > > return; > > > > + /* Bail out early on function pointers. */ > > + if (TREE_CODE (pointee_type) == FUNCTION_TYPE) > > + return; > > + > > /* Bail out early on pointers to structs where we can > > not deduce whether the buffer size is compatible. */ > > bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type); >
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index f079d1fb37e..1b43443d168 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -3514,6 +3514,10 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval, || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE) return; + /* Bail out early on function pointers. */ + if (TREE_CODE (pointee_type) == FUNCTION_TYPE) + return; + /* Bail out early on pointers to structs where we can not deduce whether the buffer size is compatible. */ bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type);