On 2 November 2016 at 23:17, Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > On 2 November 2016 at 23:07, Jason Merrill <ja...@redhat.com> wrote: >> On Wed, Nov 2, 2016 at 1:08 PM, Prathamesh Kulkarni >> <prathamesh.kulka...@linaro.org> wrote: >>> On 2 November 2016 at 18:29, Jason Merrill <ja...@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