[ping * 4] PR35503 - warn for restrict

[ping * 4] PR35503 - warn for restrict

[Prathamesh Kulkarni]

Pinging patch: https://gcc.gnu.org/ml/gcc-patches/2016-10/msg01545.html

Thanks,
Prathamesh

Re: [ping * 4] PR35503 - warn for restrict

[Joseph Myers]

The format-checking parts of the patch are OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [ping * 4] PR35503 - warn for restrict

[Jason Merrill]

Then I'll approve the whole patch.

On Wed, Nov 2, 2016 at 8:42 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> The format-checking parts of the patch are OK.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [ping * 4] PR35503 - warn for restrict

[Prathamesh Kulkarni]

On 2 November 2016 at 18:29, Jason Merrill <jason@redhat.com> wrote:
> Then I'll approve the whole patch.
Thanks!
Trying the patch on kernel build (allmodconfig) reveals the following
couple of warnings:
http://pastebin.com/Sv2HFDUv

I think warning for str_error_r() is correct, however I am not sure if
warning for
pager_preexec() is legit or a false positive:

pager.c: In function 'pager_preexec':
pager.c:35:12: warning: passing argument 2 to restrict-qualified
parameter aliases with argument 4 [-Wrestrict]
  select(1, &in, NULL, &in, NULL);
                 ^~~            ~~~
Is the warning correct for  the above call to select() syscall ?

I am a bit anxious about keeping the warning in Wall because it's
breaking the kernel.
Should we instead keep it in Wextra, or continue keeping it in Wall ?

Also is there a way to gracefully disable Werror for kernel builds ?
I tried:
make allmodconfig
make all KCFLAGS="-Wno-error=restrict" CFLAGS="-Wno-error=restrict" -j8
but that didn't work. I managed to workaround by manually modifying
Makefiles to not pass Werror,
but I hope there's a better way.

Thanks,
Prathamesh
>
> On Wed, Nov 2, 2016 at 8:42 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> The format-checking parts of the patch are OK.
>>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com

Re: [ping * 4] PR35503 - warn for restrict

[Jason Merrill]

On Wed, Nov 2, 2016 at 1:08 PM, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 2 November 2016 at 18:29, Jason Merrill <jason@redhat.com> wrote:
>> Then I'll approve the whole patch.
> Thanks!
> Trying the patch on kernel build (allmodconfig) reveals the following
> couple of warnings:
> http://pastebin.com/Sv2HFDUv
>
> I think warning for str_error_r() is correct

It's accurate, but unhelpful; snprintf isn't going to use the contents
of buf via the variadic argument, so this warning is just noise.

> however I am not sure if
> warning for pager_preexec() is legit or a false positive:
>
> pager.c: In function 'pager_preexec':
> pager.c:35:12: warning: passing argument 2 to restrict-qualified
> parameter aliases with argument 4 [-Wrestrict]
>   select(1, &in, NULL, &in, NULL);
>                  ^~~            ~~~
> Is the warning correct for  the above call to select() syscall ?

The warning looks correct based on the prototype

extern int select (int __nfds, fd_set *__restrict __readfds,
                   fd_set *__restrict __writefds,
                   fd_set *__restrict __exceptfds,
                   struct timeval *__restrict __timeout);

But passing the same fd_set to both readfds and exceptfds seems
reasonable to me, so this also seems like a false positive.

Looking at C11, I see this example:

EXAMPLE 3 The function parameter declarations
void h(int n, int * restrict p, int * restrict q, int * restrict r)
{
  int i;
  for (i = 0; i < n; i++)
    p[i] = q[i] + r[i];
}

illustrate how an unmodified object can be aliased through two
restricted pointers. In particular, if a and b
are disjoint arrays, a call of the form h(100, a, b, b) has defined
behavior, because array b is not
modified within function h.

This is is another example of well-defined code that your warning will
complain about.

> Should we instead keep it in Wextra, or continue keeping it in Wall ?

It seems that it doesn't belong in -Wall.  I don't feel strongly about -Wextra.

Jason

Re: [ping * 4] PR35503 - warn for restrict

[Prathamesh Kulkarni]

On 2 November 2016 at 23:07, Jason Merrill <jason@redhat.com> wrote:
> On Wed, Nov 2, 2016 at 1:08 PM, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>> On 2 November 2016 at 18:29, Jason Merrill <jason@redhat.com> wrote:
>>> Then I'll approve the whole patch.
>> Thanks!
>> Trying the patch on kernel build (allmodconfig) reveals the following
>> couple of warnings:
>> http://pastebin.com/Sv2HFDUv
>>
>> I think warning for str_error_r() is correct
>
> It's accurate, but unhelpful; snprintf isn't going to use the contents
> of buf via the variadic argument, so this warning is just noise.
Ah, indeed, it's just printing address of buf, not using the contents.
>
>> however I am not sure if
>> warning for pager_preexec() is legit or a false positive:
>>
>> pager.c: In function 'pager_preexec':
>> pager.c:35:12: warning: passing argument 2 to restrict-qualified
>> parameter aliases with argument 4 [-Wrestrict]
>>   select(1, &in, NULL, &in, NULL);
>>                  ^~~            ~~~
>> Is the warning correct for  the above call to select() syscall ?
>
> The warning looks correct based on the prototype
>
> extern int select (int __nfds, fd_set *__restrict __readfds,
>                    fd_set *__restrict __writefds,
>                    fd_set *__restrict __exceptfds,
>                    struct timeval *__restrict __timeout);
>
> But passing the same fd_set to both readfds and exceptfds seems
> reasonable to me, so this also seems like a false positive.
>
> Looking at C11, I see this example:
>
> EXAMPLE 3 The function parameter declarations
> void h(int n, int * restrict p, int * restrict q, int * restrict r)
> {
>   int i;
>   for (i = 0; i < n; i++)
>     p[i] = q[i] + r[i];
> }
>
> illustrate how an unmodified object can be aliased through two
> restricted pointers. In particular, if a and b
> are disjoint arrays, a call of the form h(100, a, b, b) has defined
> behavior, because array b is not
> modified within function h.
>
> This is is another example of well-defined code that your warning will
> complain about.
Yes, that's a limitation of the patch, it just looks at the prototype, and
not how the arguments are used in the function.
>
>> Should we instead keep it in Wextra, or continue keeping it in Wall ?
>
> It seems that it doesn't belong in -Wall.  I don't feel strongly about -Wextra.
Should I commit the patch by keeping Wrestrict "standalone",
ie, not including it in either Wall or Wextra ?

Thanks,
Prathamesh
>
> Jason

Re: [ping * 4] PR35503 - warn for restrict

[Prathamesh Kulkarni]

On 2 November 2016 at 23:17, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 2 November 2016 at 23:07, Jason Merrill <jason@redhat.com> wrote:
>> On Wed, Nov 2, 2016 at 1:08 PM, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>>> On 2 November 2016 at 18:29, Jason Merrill <jason@redhat.com> wrote:
>>>> Then I'll approve the whole patch.
>>> Thanks!
>>> Trying the patch on kernel build (allmodconfig) reveals the following
>>> couple of warnings:
>>> http://pastebin.com/Sv2HFDUv
>>>
>>> I think warning for str_error_r() is correct
>>
>> It's accurate, but unhelpful; snprintf isn't going to use the contents
>> of buf via the variadic argument, so this warning is just noise.
> Ah, indeed, it's just printing address of buf, not using the contents.
>>
>>> however I am not sure if
>>> warning for pager_preexec() is legit or a false positive:
>>>
>>> pager.c: In function 'pager_preexec':
>>> pager.c:35:12: warning: passing argument 2 to restrict-qualified
>>> parameter aliases with argument 4 [-Wrestrict]
>>>   select(1, &in, NULL, &in, NULL);
>>>                  ^~~            ~~~
>>> Is the warning correct for  the above call to select() syscall ?
>>
>> The warning looks correct based on the prototype
>>
>> extern int select (int __nfds, fd_set *__restrict __readfds,
>>                    fd_set *__restrict __writefds,
>>                    fd_set *__restrict __exceptfds,
>>                    struct timeval *__restrict __timeout);
>>
>> But passing the same fd_set to both readfds and exceptfds seems
>> reasonable to me, so this also seems like a false positive.
>>
>> Looking at C11, I see this example:
>>
>> EXAMPLE 3 The function parameter declarations
>> void h(int n, int * restrict p, int * restrict q, int * restrict r)
>> {
>>   int i;
>>   for (i = 0; i < n; i++)
>>     p[i] = q[i] + r[i];
>> }
>>
>> illustrate how an unmodified object can be aliased through two
>> restricted pointers. In particular, if a and b
>> are disjoint arrays, a call of the form h(100, a, b, b) has defined
>> behavior, because array b is not
>> modified within function h.
>>
>> This is is another example of well-defined code that your warning will
>> complain about.
> Yes, that's a limitation of the patch, it just looks at the prototype, and
> not how the arguments are used in the function.
>>
>>> Should we instead keep it in Wextra, or continue keeping it in Wall ?
>>
>> It seems that it doesn't belong in -Wall.  I don't feel strongly about -Wextra.
> Should I commit the patch by keeping Wrestrict "standalone",
> ie, not including it in either Wall or Wextra ?
Hi,
After Joseph and Jason's approval, I have committed a rebased version
of patch as r242366
after bootstrap+test on x86_64-unknown-linux-gnu, cross-test on
arm*-*-*, aarch64*-*-* and
verifying no warning is triggered on kernel build with make
allmodconfig && make all.
Because the patch only looks at function prototype, there could be
false positives with the warning (restrict example 3 in C11 std),
and hence the warning isn't enabled by default, and neither by Wall or Wextra.
The warning is only enabled with -Wrestrict option.

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
>>
>> Jason