public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix bogus -Wstringop-overflow warning
@ 2022-10-13 12:06 Eric Botcazou
  2022-10-13 22:31 ` Jeff Law
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2022-10-13 12:06 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi,

if you compile the attached testcase with -O2 -fno-inline -Wall, you get:

In function 'process_array3':
cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [-
Wstringop-overflow=]
cc1: note: referencing argument 1 of type 'char[4]'
t.c:6:6: note: in a call to function 'process_array4'
    6 | void process_array4 (char a[4], int n)
      |      ^~~~~~~~~~~~~~
cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [-
Wstringop-overflow=]
cc1: note: referencing argument 1 of type 'char[4]'
t.c:6:6: note: in a call to function 'process_array4'

That's because the ICF IPA pass has identified the two functions and turned 
process_array3 into a wrapper of process_array4.  This looks sensible to me 
given that the only difference between them is an "access" attribute on their 
type describing the access size of the parameter and the "access" attribute 
does not affect type identity (struct attribute_spec.affects_type_identity).

Hence the proposed fix, tested on x86-64/Linux, OK for the mainline?


2022-10-13  Eric Botcazou  <ebotcazou@adacore.com>

	* gimple-ssa-warn-access.cc (pass_waccess::check_call): Return
	early for calls made from thunks.


2022-10-13  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/Wstringop-overflow-89.c: New test.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 862 bytes --]

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 04aa849a4b1..59a70530600 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4291,14 +4291,18 @@ pass_waccess::check_pointer_uses (gimple *stmt, tree ptr,
 void
 pass_waccess::check_call (gcall *stmt)
 {
-  if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
-    check_builtin (stmt);
+  /* Skip special calls generated by the compiler.  */
+  if (gimple_call_from_thunk_p (stmt))
+    return;
 
   /* .ASAN_MARK doesn't access any vars, only modifies shadow memory.  */
   if (gimple_call_internal_p (stmt)
       && gimple_call_internal_fn (stmt) == IFN_ASAN_MARK)
     return;
 
+  if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
+    check_builtin (stmt);
+
   if (!m_early_checks_p)
     if (tree callee = gimple_call_fndecl (stmt))
       {

[-- Attachment #3: Wstringop-overflow-89.c --]
[-- Type: text/x-csrc, Size: 286 bytes --]

/* { dg-do compile } */
/* { dg-options "-O2 -fno-inline -Wall" } */

extern void process (char);

void process_array4 (char a[4], int n)
{
  for (int i = 0; i < n; i++)
    process (a[i]);
}

void process_array3 (char a[3], int n)
{
  for (int i = 0; i < n; i++)
    process (a[i]);
}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix bogus -Wstringop-overflow warning
  2022-10-13 12:06 [PATCH] Fix bogus -Wstringop-overflow warning Eric Botcazou
@ 2022-10-13 22:31 ` Jeff Law
  2022-10-13 22:53   ` Eric Botcazou
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2022-10-13 22:31 UTC (permalink / raw)
  To: Eric Botcazou, gcc-patches


On 10/13/22 06:06, Eric Botcazou via Gcc-patches wrote:
> Hi,
>
> if you compile the attached testcase with -O2 -fno-inline -Wall, you get:
>
> In function 'process_array3':
> cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [-
> Wstringop-overflow=]
> cc1: note: referencing argument 1 of type 'char[4]'
> t.c:6:6: note: in a call to function 'process_array4'
>      6 | void process_array4 (char a[4], int n)
>        |      ^~~~~~~~~~~~~~
> cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [-
> Wstringop-overflow=]
> cc1: note: referencing argument 1 of type 'char[4]'
> t.c:6:6: note: in a call to function 'process_array4'
>
> That's because the ICF IPA pass has identified the two functions and turned
> process_array3 into a wrapper of process_array4.  This looks sensible to me
> given that the only difference between them is an "access" attribute on their
> type describing the access size of the parameter and the "access" attribute
> does not affect type identity (struct attribute_spec.affects_type_identity).
>
> Hence the proposed fix, tested on x86-64/Linux, OK for the mainline?
>
>
> 2022-10-13  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* gimple-ssa-warn-access.cc (pass_waccess::check_call): Return
> 	early for calls made from thunks.
>
>
> 2022-10-13  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* gcc.dg/Wstringop-overflow-89.c: New test.

Not a fan as it could potentially hide a real issue, but I don't really 
have a better solution.  I pondered suggesting "access" affect type 
identity, but the cases where that's really important are probably 
better handled by the "fn spec" attribute, leaving "access" strictly 
impacting diagnostics.

OK

jeff


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix bogus -Wstringop-overflow warning
  2022-10-13 22:31 ` Jeff Law
@ 2022-10-13 22:53   ` Eric Botcazou
  2022-10-14  6:12     ` Richard Biener
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2022-10-13 22:53 UTC (permalink / raw)
  To: Jeff Law; +Cc: gcc-patches

> Not a fan as it could potentially hide a real issue, but I don't really
> have a better solution.

Thanks.

> I pondered suggesting "access" affect type identity, but the cases where
> that's really important are probably better handled by the "fn spec"
> attribute, leaving "access" strictly impacting diagnostics.

I can expand a bit here, because I tried to change the "access" attribute that 
way and this badly breaks the C compiler, for example:

int foo (int n, char m[1][n]);

int foo (int n, char m[1][n]) {}

no longer compiles with an error about different function types.

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix bogus -Wstringop-overflow warning
  2022-10-13 22:53   ` Eric Botcazou
@ 2022-10-14  6:12     ` Richard Biener
  2022-10-14 10:27       ` Martin Liška
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2022-10-14  6:12 UTC (permalink / raw)
  To: Eric Botcazou, Martin Liška; +Cc: Jeff Law, gcc-patches

On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > Not a fan as it could potentially hide a real issue, but I don't really
> > have a better solution.
>
> Thanks.
>
> > I pondered suggesting "access" affect type identity, but the cases where
> > that's really important are probably better handled by the "fn spec"
> > attribute, leaving "access" strictly impacting diagnostics.
>
> I can expand a bit here, because I tried to change the "access" attribute that
> way and this badly breaks the C compiler, for example:
>
> int foo (int n, char m[1][n]);
>
> int foo (int n, char m[1][n]) {}
>
> no longer compiles with an error about different function types.

Note in discussion with IPA folks we agreed that IPA cloning that modifies
arguments either has to remove access attributes, adjust them or refrain
from cloning.

Martin - has anything been done to this respect?

I suppose there's also a way to figure if a clone has arguments
changed in any way?

Thanks,
Richard.

> --
> Eric Botcazou
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix bogus -Wstringop-overflow warning
  2022-10-14  6:12     ` Richard Biener
@ 2022-10-14 10:27       ` Martin Liška
  2022-10-14 11:59         ` Martin Jambor
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Liška @ 2022-10-14 10:27 UTC (permalink / raw)
  To: Richard Biener, Eric Botcazou; +Cc: Jeff Law, gcc-patches, Martin Jambor

On 10/14/22 08:12, Richard Biener wrote:
> On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>> Not a fan as it could potentially hide a real issue, but I don't really
>>> have a better solution.
>>
>> Thanks.
>>
>>> I pondered suggesting "access" affect type identity, but the cases where
>>> that's really important are probably better handled by the "fn spec"
>>> attribute, leaving "access" strictly impacting diagnostics.
>>
>> I can expand a bit here, because I tried to change the "access" attribute that
>> way and this badly breaks the C compiler, for example:
>>
>> int foo (int n, char m[1][n]);
>>
>> int foo (int n, char m[1][n]) {}
>>
>> no longer compiles with an error about different function types.
> 
> Note in discussion with IPA folks we agreed that IPA cloning that modifies
> arguments either has to remove access attributes, adjust them or refrain
> from cloning.
> 
> Martin - has anything been done to this respect?

I think it's more for Martin Jambor who's the IPA specialist when it comes
to parameter manipulation.

Martin

> 
> I suppose there's also a way to figure if a clone has arguments
> changed in any way?
> 
> Thanks,
> Richard.
> 
>> --
>> Eric Botcazou
>>
>>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix bogus -Wstringop-overflow warning
  2022-10-14 10:27       ` Martin Liška
@ 2022-10-14 11:59         ` Martin Jambor
  0 siblings, 0 replies; 6+ messages in thread
From: Martin Jambor @ 2022-10-14 11:59 UTC (permalink / raw)
  To: Martin Liška, Richard Biener, Eric Botcazou; +Cc: Jeff Law, gcc-patches

Hello,

On Fri, Oct 14 2022, Martin Liška wrote:
> On 10/14/22 08:12, Richard Biener wrote:
>> On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>> Not a fan as it could potentially hide a real issue, but I don't really
>>>> have a better solution.
>>>
>>> Thanks.
>>>
>>>> I pondered suggesting "access" affect type identity, but the cases where
>>>> that's really important are probably better handled by the "fn spec"
>>>> attribute, leaving "access" strictly impacting diagnostics.
>>>
>>> I can expand a bit here, because I tried to change the "access" attribute that
>>> way and this badly breaks the C compiler, for example:
>>>
>>> int foo (int n, char m[1][n]);
>>>
>>> int foo (int n, char m[1][n]) {}
>>>
>>> no longer compiles with an error about different function types.
>> 
>> Note in discussion with IPA folks we agreed that IPA cloning that modifies
>> arguments either has to remove access attributes, adjust them or refrain
>> from cloning.
>> 
>> Martin - has anything been done to this respect?
>
> I think it's more for Martin Jambor who's the IPA specialist when it comes
> to parameter manipulation.
>

They are being dropped since 2af63f0f53a

Adjusting them accordingly is an item buried quite deep in my TODO list.

>> 
>> I suppose there's also a way to figure if a clone has arguments
>> changed in any way?

Look whether clone_info::get (node) exists and its param_adjustments is
non-NULL.

In theory the param_adjustments could contain description of the very
same signature the original function has but in practice it does not
currently happen and is unlikely to happen ever.

Martin

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-10-14 11:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-13 12:06 [PATCH] Fix bogus -Wstringop-overflow warning Eric Botcazou
2022-10-13 22:31 ` Jeff Law
2022-10-13 22:53   ` Eric Botcazou
2022-10-14  6:12     ` Richard Biener
2022-10-14 10:27       ` Martin Liška
2022-10-14 11:59         ` Martin Jambor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).