analyzer: Bail out on function pointer for -Wanalyzer-allocation-size
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Testing passed
|
Commit Message
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.
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?
---
gcc/analyzer/region-model.cc | 4 ++++
1 file changed, 4 insertions(+)
Comments
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);
>
@@ -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);