| Message ID | 20171218151022.GL2605@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PATCH to fix -Wrestrict ICE (PR middle-end/83463) | expand |
On 12/18/2017 08:10 AM, Marek Polacek wrote: > I'm not entirely up to speed with this code, but this one seemed sufficiently > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. > Otherwise, go with maxobjsize. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2017-12-18 Marek Polacek <polacek@redhat.com> > > PR middle-end/83463 > * gimple-ssa-warn-restrict.c (builtin_memref::builtin_memref): > Check if TYPE is INTEGRAL_TYPE_P before accessing its min/max > values. > > * gcc.dg/pr83463.c: New test. OK. jeff
On Mon, Dec 18, 2017 at 09:36:46AM -0700, Jeff Law wrote: > On 12/18/2017 08:10 AM, Marek Polacek wrote: > > I'm not entirely up to speed with this code, but this one seemed sufficiently > > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. > > Otherwise, go with maxobjsize. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > 2017-12-18 Marek Polacek <polacek@redhat.com> > > > > PR middle-end/83463 > > * gimple-ssa-warn-restrict.c (builtin_memref::builtin_memref): > > Check if TYPE is INTEGRAL_TYPE_P before accessing its min/max > > values. > > > > * gcc.dg/pr83463.c: New test. > OK. What about INTEGRAL_TYPE_Ps that have NULL TYPE_MIN_VALUE and/or TYPE_MAX_VALUE? Doesn't Ada create those? Jakub
On 12/18/2017 08:10 AM, Marek Polacek wrote: > I'm not entirely up to speed with this code, but this one seemed sufficiently > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. > Otherwise, go with maxobjsize. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Thanks for the fix. I noticed the problem only comes up when memcpy is declared without a prototype. Otherwise, the memcpy call is eliminated early on (during gimplification?) So while avoiding the ICE is obviously a necessary change, I wonder if this bug also suggests a missing optimization that might still be relevant (i.e., eliminating memcpy() with a zero size for a memcpy without a prototype): void* memcpy (); void p (void *d, const void *s) { memcpy (d, s, 0); } GCC doesn't warn for this code which means it views the memcpy as a built-in but it doesn't eliminate the call. If I declare memcpy for example like this, it complains about the mismatch but it does eliminates the call: void* memcpy (long, long, long); Martin > > 2017-12-18 Marek Polacek <polacek@redhat.com> > > PR middle-end/83463 > * gimple-ssa-warn-restrict.c (builtin_memref::builtin_memref): > Check if TYPE is INTEGRAL_TYPE_P before accessing its min/max > values. > > * gcc.dg/pr83463.c: New test. > > diff --git gcc/gimple-ssa-warn-restrict.c gcc/gimple-ssa-warn-restrict.c > index 4d424735d2a..d1a376637a2 100644 > --- gcc/gimple-ssa-warn-restrict.c > +++ gcc/gimple-ssa-warn-restrict.c > @@ -287,13 +287,15 @@ builtin_memref::builtin_memref (tree expr, tree size) > else > { > gimple *stmt = SSA_NAME_DEF_STMT (offset); > + tree type; > if (is_gimple_assign (stmt) > - && gimple_assign_rhs_code (stmt) == NOP_EXPR) > + && gimple_assign_rhs_code (stmt) == NOP_EXPR > + && (type = TREE_TYPE (gimple_assign_rhs1 (stmt))) > + && INTEGRAL_TYPE_P (type)) > { > /* Use the bounds of the type of the NOP_EXPR operand > even if it's signed. The result doesn't trigger > warnings but makes their output more readable. */ > - tree type = TREE_TYPE (gimple_assign_rhs1 (stmt)); > offrange[0] = wi::to_offset (TYPE_MIN_VALUE (type)); > offrange[1] = wi::to_offset (TYPE_MAX_VALUE (type)); > } > diff --git gcc/testsuite/gcc.dg/pr83463.c gcc/testsuite/gcc.dg/pr83463.c > index e69de29bb2d..735ef3c6dc8 100644 > --- gcc/testsuite/gcc.dg/pr83463.c > +++ gcc/testsuite/gcc.dg/pr83463.c > @@ -0,0 +1,17 @@ > +/* PR middle-end/83463 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -Wrestrict" } */ > + > +int *a; > +void *memcpy (); > +void > +m (void *p1) > +{ > + memcpy (0, p1, 0); > +} > + > +void > +p () > +{ > + m (p + (long) a); > +} > > Marek >
On Mon, Dec 18, 2017 at 10:00:36AM -0700, Martin Sebor wrote: > On 12/18/2017 08:10 AM, Marek Polacek wrote: > > I'm not entirely up to speed with this code, but this one seemed sufficiently > > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. > > Otherwise, go with maxobjsize. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > Thanks for the fix. I noticed the problem only comes up when > memcpy is declared without a prototype. Otherwise, the memcpy > call is eliminated early on (during gimplification?) So while > avoiding the ICE is obviously a necessary change, I wonder if > this bug also suggests a missing optimization that might still > be relevant (i.e., eliminating memcpy() with a zero size for > a memcpy without a prototype): No, we shouldn't try to optimize if people mess up stuff. What happens in this case is that gimple_builtin_call_types_compatible_p will fail, thus gimple_call_builtin_p and we won't be treating it specially. That just shows a problem in the new pass: tree func = gimple_call_fndecl (call); if (!func || DECL_BUILT_IN_CLASS (func) != BUILT_IN_NORMAL) return; is just wrong, it should have been: if (!gimple_call_builtin_p (call, BUILT_IN_NORMAL)) return; Then you don't need to worry about float or _Complex int third argument, or just 2 or 27 arguments instead of 3 for memcpy, etc. Jakub
On 12/18/2017 10:07 AM, Jakub Jelinek wrote: > On Mon, Dec 18, 2017 at 10:00:36AM -0700, Martin Sebor wrote: >> On 12/18/2017 08:10 AM, Marek Polacek wrote: >>> I'm not entirely up to speed with this code, but this one seemed sufficiently >>> obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. >>> Otherwise, go with maxobjsize. >>> >>> Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> Thanks for the fix. I noticed the problem only comes up when >> memcpy is declared without a prototype. Otherwise, the memcpy >> call is eliminated early on (during gimplification?) So while >> avoiding the ICE is obviously a necessary change, I wonder if >> this bug also suggests a missing optimization that might still >> be relevant (i.e., eliminating memcpy() with a zero size for >> a memcpy without a prototype): > > No, we shouldn't try to optimize if people mess up stuff. Declaring memcpy() without a prototype isn't messing up. It may be poor practice but there's nothing inherently wrong with it. GCC accepts it, treats it as a built-in (i.e., doesn't complain about the declaration being incompatible with the built-in), but doesn't optimize it. On the other hand, as I said, when someone really does mess up and declares memcpy like so: void* memcpy (long, long, long); GCC optimizes calls to it as if the function were declared with the right prototype. > What happens in this case is that > gimple_builtin_call_types_compatible_p > will fail, thus gimple_call_builtin_p and we won't be treating it specially. > That just shows a problem in the new pass: > tree func = gimple_call_fndecl (call); > if (!func || DECL_BUILT_IN_CLASS (func) != BUILT_IN_NORMAL) > return; > is just wrong, it should have been: > if (!gimple_call_builtin_p (call, BUILT_IN_NORMAL)) > return; > > Then you don't need to worry about float or _Complex int third argument, > or just 2 or 27 arguments instead of 3 for memcpy, etc. Yes, but only at the cost of not checking calls to memcpy() and other built ins declared without a prototype. I didn't consider those when I wrote the code but now that I have, since GCC allows built-ins to be declared without a prototype and doesn't warn even with -Wpedantic, it doesn't seem like a good approach to me. Martin
On Mon, Dec 18, 2017 at 10:37:19AM -0700, Martin Sebor wrote: > On 12/18/2017 10:07 AM, Jakub Jelinek wrote: > > On Mon, Dec 18, 2017 at 10:00:36AM -0700, Martin Sebor wrote: > > > On 12/18/2017 08:10 AM, Marek Polacek wrote: > > > > I'm not entirely up to speed with this code, but this one seemed sufficiently > > > > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. > > > > Otherwise, go with maxobjsize. > > > > > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > > > Thanks for the fix. I noticed the problem only comes up when > > > memcpy is declared without a prototype. Otherwise, the memcpy > > > call is eliminated early on (during gimplification?) So while > > > avoiding the ICE is obviously a necessary change, I wonder if > > > this bug also suggests a missing optimization that might still > > > be relevant (i.e., eliminating memcpy() with a zero size for > > > a memcpy without a prototype): > > > > No, we shouldn't try to optimize if people mess up stuff. > > Declaring memcpy() without a prototype isn't messing up. It may > be poor practice but there's nothing inherently wrong with it. There is. Because the last argument is size_t, so calling it with 0 is UB (on LP64, on ILP32 I think it will be optimized normally). Jakub
On 12/18/2017 10:45 AM, Jakub Jelinek wrote: > On Mon, Dec 18, 2017 at 10:37:19AM -0700, Martin Sebor wrote: >> On 12/18/2017 10:07 AM, Jakub Jelinek wrote: >>> On Mon, Dec 18, 2017 at 10:00:36AM -0700, Martin Sebor wrote: >>>> On 12/18/2017 08:10 AM, Marek Polacek wrote: >>>>> I'm not entirely up to speed with this code, but this one seemed sufficiently >>>>> obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value. >>>>> Otherwise, go with maxobjsize. >>>>> >>>>> Bootstrapped/regtested on x86_64-linux, ok for trunk? >>>> >>>> Thanks for the fix. I noticed the problem only comes up when >>>> memcpy is declared without a prototype. Otherwise, the memcpy >>>> call is eliminated early on (during gimplification?) So while >>>> avoiding the ICE is obviously a necessary change, I wonder if >>>> this bug also suggests a missing optimization that might still >>>> be relevant (i.e., eliminating memcpy() with a zero size for >>>> a memcpy without a prototype): >>> >>> No, we shouldn't try to optimize if people mess up stuff. >> >> Declaring memcpy() without a prototype isn't messing up. It may >> be poor practice but there's nothing inherently wrong with it. > > There is. Because the last argument is size_t, so calling it with 0 > is UB (on LP64, on ILP32 I think it will be optimized normally). It isn't optimized either way. In fact, the only indication of a problem in the code below is the new -Wrestrict warning: void* memcpy (); void p () { memcpy (0, 0, 0); } warning: ‘memcpy’ source argument is the same as destination [-Wrestrict] memcpy (0, 0, 0); ^~~~~~~~~~~~~~~~ ;; Function p (p, funcdef_no=0, decl_uid=1892, cgraph_uid=0, symbol_order=0) p () { <bb 2> [local count: 1073741825]: memcpy (0, 0, 0); [tail call] return; } But you are conflating the ability to call such a built-in with arguments of the wrong types with declaring it. There really is nothing wrong with declaring it as long as GCC checks the calls. To ameliorate the undefined behavior you point out, GCC could do one of three things: a) complain about declaring a built-in with no prototype, or b) complain about calls to such built-ins that pass arguments of the wrong type (or the wrong number of arguments), or c) convert the arguments to the expected type and still warn on mismatches). In all cases all the information necessary to detect and diagnose or even avoid the problem is available. In fact, one might argue that optimizing such calls (expanding them inline) would be preferable to doing nothing and allowing the undefined behavior to cause a bug at runtime. Martin
On Mon, Dec 18, 2017 at 11:08:19AM -0700, Martin Sebor wrote: > It isn't optimized either way. In fact, the only indication > of a problem in the code below is the new -Wrestrict warning: So just call it as memcpy (0, 0, (size_t) 0); or memcpy (0, 0, 0UL); on targets where size_t is unsigned long int, and it will be optimized away, because it will be compatible with the builtin's prototype then. gimple_call_builtin_p is the API that has been agreed upon to be used for checking for builtins several years ago, there is absolutely no reason why this pass needs to treat them differently. Jakub
On 12/18/2017 12:04 PM, Jakub Jelinek wrote: > On Mon, Dec 18, 2017 at 11:08:19AM -0700, Martin Sebor wrote: >> It isn't optimized either way. In fact, the only indication >> of a problem in the code below is the new -Wrestrict warning: > > So just call it as memcpy (0, 0, (size_t) 0); or memcpy (0, 0, 0UL); > on targets where size_t is unsigned long int, and it will be optimized away, > because it will be compatible with the builtin's prototype then. A call to memcpy(0, 0, (size_t)0) is not eliminated on any target, presumably because the type of 0 is int and not void*. "Don't write bad code" is also not an answer to the question of why shouldn't GCC eliminate the bad code (null pointer) when there is no prototype given that it eliminates it when the function is declared with one. But I'm not insisting on it. What I think will much more helpful is diagnosing such bad calls, similarly to how attribute sentinel diagnoses the analogous subset of the problem involving variadic functions. > gimple_call_builtin_p is the API that has been agreed upon to be used for > checking for builtins several years ago, there is absolutely no reason why > this pass needs to treat them differently. I already explained the reason: to help detect bugs. That's the essential purpose of the pass (although this particular bug wasn't necessarily its original focus). Using an API that would result in compromising this goal without offering other benefits would be self-defeating. Martin
On Mon, Dec 18, 2017 at 04:04:11PM -0700, Martin Sebor wrote: > On 12/18/2017 12:04 PM, Jakub Jelinek wrote: > > On Mon, Dec 18, 2017 at 11:08:19AM -0700, Martin Sebor wrote: > > > It isn't optimized either way. In fact, the only indication > > > of a problem in the code below is the new -Wrestrict warning: > > > > So just call it as memcpy (0, 0, (size_t) 0); or memcpy (0, 0, 0UL); > > on targets where size_t is unsigned long int, and it will be optimized away, > > because it will be compatible with the builtin's prototype then. > > A call to memcpy(0, 0, (size_t)0) is not eliminated on any target, > presumably because the type of 0 is int and not void*. "Don't > write bad code" is also not an answer to the question of why > shouldn't GCC eliminate the bad code (null pointer) when there > is no prototype given that it eliminates it when the function > is declared with one. But I'm not insisting on it. What > I think will much more helpful is diagnosing such bad calls, > similarly to how attribute sentinel diagnoses the analogous > subset of the problem involving variadic functions. > > > gimple_call_builtin_p is the API that has been agreed upon to be used for > > checking for builtins several years ago, there is absolutely no reason why > > this pass needs to treat them differently. > > I already explained the reason: to help detect bugs. That's Your warning is about restrict and argument overlap, what does it have to do with unprototyped calls? Nothing. There is no restrict in that case, and it isn't handled as builtin if it doesn't match the builtin's prototype. The user can use -Wstrict-prototypes and -Wbuiltin-declaration-mismatch already, and if needed, we can resurrect the "Add new warning -Wunprototyped-calls" thread for the rest. That is actually that can help (the remaining few users that actually still use unprototyped calls). Jakub
On 12/18/2017 04:41 PM, Jakub Jelinek wrote: > On Mon, Dec 18, 2017 at 04:04:11PM -0700, Martin Sebor wrote: >> On 12/18/2017 12:04 PM, Jakub Jelinek wrote: >>> On Mon, Dec 18, 2017 at 11:08:19AM -0700, Martin Sebor wrote: >>>> It isn't optimized either way. In fact, the only indication >>>> of a problem in the code below is the new -Wrestrict warning: >>> >>> So just call it as memcpy (0, 0, (size_t) 0); or memcpy (0, 0, 0UL); >>> on targets where size_t is unsigned long int, and it will be optimized away, >>> because it will be compatible with the builtin's prototype then. >> >> A call to memcpy(0, 0, (size_t)0) is not eliminated on any target, >> presumably because the type of 0 is int and not void*. "Don't >> write bad code" is also not an answer to the question of why >> shouldn't GCC eliminate the bad code (null pointer) when there >> is no prototype given that it eliminates it when the function >> is declared with one. But I'm not insisting on it. What >> I think will much more helpful is diagnosing such bad calls, >> similarly to how attribute sentinel diagnoses the analogous >> subset of the problem involving variadic functions. >> >>> gimple_call_builtin_p is the API that has been agreed upon to be used for >>> checking for builtins several years ago, there is absolutely no reason why >>> this pass needs to treat them differently. >> >> I already explained the reason: to help detect bugs. That's > > Your warning is about restrict and argument overlap, what does it have to do > with unprototyped calls? Nothing. There is no restrict in that case, and > it isn't handled as builtin if it doesn't match the builtin's prototype. $ cat a.c && gcc -O2 -S -Wrestrict a.c void* memcpy (); char a[5]; void f (void) { memcpy (a, a + 1, 3); } a.c: In function ‘f’: a.c:7:3: warning: ‘memcpy’ accessing 3 bytes at offsets 0 and 1 overlaps 2 bytes at offset 1 [-Wrestrict] memcpy (a, a + 1, 3); ^~~~~~~~~~~~~~~~~~~~ By insisting on using gimple_call_builtin_p() you're effectively arguing to disable the warning above, for no good reason that I can see. I'm happy to take suggestions to improve the pass and fix bugs in it. But I see no point in debating whether it should be changed in ways that would prevent it from diagnosing bugs like the one above. That would be counter-productive. Martin
On Mon, Dec 18, 2017 at 05:03:22PM -0700, Martin Sebor wrote: > > Your warning is about restrict and argument overlap, what does it have to do > > with unprototyped calls? Nothing. There is no restrict in that case, and > > it isn't handled as builtin if it doesn't match the builtin's prototype. > > $ cat a.c && gcc -O2 -S -Wrestrict a.c > void* memcpy (); > > char a[5]; > > void f (void) > { > memcpy (a, a + 1, 3); > } > a.c: In function ‘f’: > a.c:7:3: warning: ‘memcpy’ accessing 3 bytes at offsets 0 and 1 overlaps 2 > bytes at offset 1 [-Wrestrict] > memcpy (a, a + 1, 3); > ^~~~~~~~~~~~~~~~~~~~ > > By insisting on using gimple_call_builtin_p() you're effectively > arguing to disable the warning above, for no good reason that I > can see. Because it is wrong. Consider e.g. void *mempcpy (); void foo (int *a) { mempcpy (a, (float *) a + 1, ' '); } mempcpy isn't defined in ISO C99, so it is fine if you define it yourself with void *mempcpy (int *, float *, char); prototype and do whatever you want in there. Warning about restrict doesn't make sense in that case. Jakub
On 12/18/2017 05:27 PM, Jakub Jelinek wrote: > On Mon, Dec 18, 2017 at 05:03:22PM -0700, Martin Sebor wrote: >>> Your warning is about restrict and argument overlap, what does it have to do >>> with unprototyped calls? Nothing. There is no restrict in that case, and >>> it isn't handled as builtin if it doesn't match the builtin's prototype. >> >> $ cat a.c && gcc -O2 -S -Wrestrict a.c >> void* memcpy (); >> >> char a[5]; >> >> void f (void) >> { >> memcpy (a, a + 1, 3); >> } >> a.c: In function ‘f’: >> a.c:7:3: warning: ‘memcpy’ accessing 3 bytes at offsets 0 and 1 overlaps 2 >> bytes at offset 1 [-Wrestrict] >> memcpy (a, a + 1, 3); >> ^~~~~~~~~~~~~~~~~~~~ >> >> By insisting on using gimple_call_builtin_p() you're effectively >> arguing to disable the warning above, for no good reason that I >> can see. > > Because it is wrong. I disagree. Memcpy is a reserved external identifier and its semantics are specified by the standard. A program that defines its own memcpy is undefined, Even with -ffreestanding, GCC makes assumptions about environments providing conforming memcpy and memset. Most of the other built-ins handled by the pass are also reserved by C, same as memcpy. But you know this at least as well as I do. > Consider e.g. > void *mempcpy (); > > void > foo (int *a) > { > mempcpy (a, (float *) a + 1, ' '); > } > > mempcpy isn't defined in ISO C99, so it is fine if you define it yourself > with void *mempcpy (int *, float *, char); prototype and do whatever you > want in there. Warning about restrict doesn't make sense in that case. Programs that define their own versions of GCC built-ins, whether standard C or specified elsewhere, should disable the built-ins from being recognized. That's well understood, especially thanks to -Wbuiltin-declaration-mismatch. Otherwise, as the manual says, they "may be handled as built-in functions." As I already explained, the purpose of this pass is to help detect bugs due to "honest" mistakes. I see no value in trying to disable such detection on the basis hypothetical use cases. Doing so would defeat the purpose of the pass. I'm not at all concerned about programs that don't disable built-ins but define functions with the same names yet conflicting semantics, and on top of it declare them without a prototype. If those get diagnosed, so much the better. It will be a reminder to their authors to either use -fno-builtin-xxx to disable the conflicting built-ins, at least until -Wbuiltin-declaration-mismatch is enhanced to diagnose such declarations. With that said, while I value your input, I do not see the point in continuing to debate whether the pass should diagnose these cases. In my mind the benefits are obvious and the downsides are none. Martin
diff --git gcc/gimple-ssa-warn-restrict.c gcc/gimple-ssa-warn-restrict.c index 4d424735d2a..d1a376637a2 100644 --- gcc/gimple-ssa-warn-restrict.c +++ gcc/gimple-ssa-warn-restrict.c @@ -287,13 +287,15 @@ builtin_memref::builtin_memref (tree expr, tree size) else { gimple *stmt = SSA_NAME_DEF_STMT (offset); + tree type; if (is_gimple_assign (stmt) - && gimple_assign_rhs_code (stmt) == NOP_EXPR) + && gimple_assign_rhs_code (stmt) == NOP_EXPR + && (type = TREE_TYPE (gimple_assign_rhs1 (stmt))) + && INTEGRAL_TYPE_P (type)) { /* Use the bounds of the type of the NOP_EXPR operand even if it's signed. The result doesn't trigger warnings but makes their output more readable. */ - tree type = TREE_TYPE (gimple_assign_rhs1 (stmt)); offrange[0] = wi::to_offset (TYPE_MIN_VALUE (type)); offrange[1] = wi::to_offset (TYPE_MAX_VALUE (type)); } diff --git gcc/testsuite/gcc.dg/pr83463.c gcc/testsuite/gcc.dg/pr83463.c index e69de29bb2d..735ef3c6dc8 100644 --- gcc/testsuite/gcc.dg/pr83463.c +++ gcc/testsuite/gcc.dg/pr83463.c @@ -0,0 +1,17 @@ +/* PR middle-end/83463 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wrestrict" } */ + +int *a; +void *memcpy (); +void +m (void *p1) +{ + memcpy (0, p1, 0); +} + +void +p () +{ + m (p + (long) a); +}