Re: PR35503 - warn for restrict pointer (-Wrestrict)

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Eric Gallager]

On 8/28/16, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote:
> On 26 August 2016 at 21:25, Jason Merrill <jason@redhat.com> wrote:
>> On Fri, Aug 26, 2016 at 7:39 AM, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>>> However with C++FE it appears TYPE_RESTRICT is not set for the
>>> parameters (buf and fmt)
>>> and hence the warning doesn't get emitted for C++.
>>> C FE sets TYPE_RESTRICT for them. I am not sure how to workaround this
>>> issue, and would be grateful for suggestions.
>>
>> In the C++ FE you can see TYPE_RESTRICT on the types of the
>> DECL_ARGUMENTS of the function.
> Thanks for the suggestions, I could indeed see TYPE_RESTRICT set on
> DECL_ARGUMENTS.
> The attached patch warns for both C and C++ with -Wrestrict, and I
> have put it under -Wall.
> I suppose we don't want to warn for null pointer args ?
> for eg:
> void f(int *restrict x, int *y);
>
> void foo(void)
> {
>   f(NULL, NULL) ?
> }
>
> However I suppose warning for pointer constants other than zero is desirable
> ?
> I am not sure whether restrict is valid for obj-c/obj-c++, so I have
> just kept it to C and C++
> in the patch. Should the warning also be included for obj-c and obj-c++ FE's
> ?
> Boostrapped and tested on x86_64-unknown-linux-gnu.
> OK to commit ?
>
> Thanks,
> Prathamesh
>


I tried this patch on my fork of gdb-binutils and got a few warnings
from it. Would it be possible to have the caret point to the argument
mentioned, instead of the function name? And also print the option
name? E.g., instead of the current:

or32-opc.c: In function ‘or32_print_register’:
or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
parameter aliases with argument 3
   sprintf (disassembled, "%sr%d", disassembled, regnum);
   ^~~~~~~

could it look like:

or32-opc.c: In function ‘or32_print_register’:
or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
parameter aliases with argument 3 [-Wrestrict]
   sprintf (disassembled, "%sr%d", disassembled, regnum);
            ^~~~~~~~~~~~

instead?
Just an idea. Either way, I hope this patch goes in.
Thanks,
Eric

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Marek Polacek]

On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
> I tried this patch on my fork of gdb-binutils and got a few warnings
> from it. Would it be possible to have the caret point to the argument
> mentioned, instead of the function name? And also print the option
> name? E.g., instead of the current:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3
>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>    ^~~~~~~
> 
> could it look like:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3 [-Wrestrict]
>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>             ^~~~~~~~~~~~
> 
> instead?

I didn't try to implement it, but I think this should be fairly easy to
achieve in the C FE, because c_parser_postfix_expression_after_primary
has arg_loc, which is a vector of parameter locations.

	Marek

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Jason Merrill]

On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>> I tried this patch on my fork of gdb-binutils and got a few warnings
>> from it. Would it be possible to have the caret point to the argument
>> mentioned, instead of the function name? And also print the option
>> name? E.g., instead of the current:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3
>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>    ^~~~~~~
>>
>> could it look like:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3 [-Wrestrict]
>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>             ^~~~~~~~~~~~
>>
>> instead?
>
> I didn't try to implement it, but I think this should be fairly easy to
> achieve in the C FE, because c_parser_postfix_expression_after_primary
> has arg_loc, which is a vector of parameter locations.

The C++ FE doesn't have this currently, but it could be added without
too much trouble: in cp_parser_parenthesized_expression_list, extract
the locations from the cp_expr return value of
cp_parser_assignment_expression, and then pass the locations back up
to cp_parser_postfix_expression.

Jason

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Eric Gallager]

On 8/29/16, Jason Merrill <jason@redhat.com> wrote:
> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>>> I tried this patch on my fork of gdb-binutils and got a few warnings
>>> from it. Would it be possible to have the caret point to the argument
>>> mentioned, instead of the function name? And also print the option
>>> name? E.g., instead of the current:
>>>
>>> or32-opc.c: In function ‘or32_print_register’:
>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>> parameter aliases with argument 3
>>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>    ^~~~~~~
>>>
>>> could it look like:
>>>
>>> or32-opc.c: In function ‘or32_print_register’:
>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>> parameter aliases with argument 3 [-Wrestrict]
>>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>             ^~~~~~~~~~~~
>>>
>>> instead?
>>
>> I didn't try to implement it, but I think this should be fairly easy to
>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>> has arg_loc, which is a vector of parameter locations.
>
> The C++ FE doesn't have this currently, but it could be added without
> too much trouble: in cp_parser_parenthesized_expression_list, extract
> the locations from the cp_expr return value of
> cp_parser_assignment_expression, and then pass the locations back up
> to cp_parser_postfix_expression.
>
> Jason
>


On the topic of how to get this warning working with various
frontends, is there any reason why the Objective C frontend doesn't
handle -Wrestrict? Currently when trying to use it, it just says:

cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
but not for ObjC

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Prathamesh Kulkarni]

On 30 August 2016 at 17:11, Eric Gallager <egall@gwmail.gwu.edu> wrote:
> On 8/29/16, Jason Merrill <jason@redhat.com> wrote:
>> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>>>> I tried this patch on my fork of gdb-binutils and got a few warnings
>>>> from it. Would it be possible to have the caret point to the argument
>>>> mentioned, instead of the function name? And also print the option
>>>> name? E.g., instead of the current:
>>>>
>>>> or32-opc.c: In function ‘or32_print_register’:
>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>> parameter aliases with argument 3
>>>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>    ^~~~~~~
>>>>
>>>> could it look like:
>>>>
>>>> or32-opc.c: In function ‘or32_print_register’:
>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>> parameter aliases with argument 3 [-Wrestrict]
>>>>    sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>             ^~~~~~~~~~~~
>>>>
>>>> instead?
>>>
>>> I didn't try to implement it, but I think this should be fairly easy to
>>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>>> has arg_loc, which is a vector of parameter locations.
>>
>> The C++ FE doesn't have this currently, but it could be added without
>> too much trouble: in cp_parser_parenthesized_expression_list, extract
>> the locations from the cp_expr return value of
>> cp_parser_assignment_expression, and then pass the locations back up
>> to cp_parser_postfix_expression.
>>
>> Jason
>>
>
>
> On the topic of how to get this warning working with various
> frontends, is there any reason why the Objective C frontend doesn't
> handle -Wrestrict? Currently when trying to use it, it just says:
>
> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
> but not for ObjC
Hi Eric,
I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
add the option for these front-ends.
If it is valid, I will enable the option for ObjC and Obj-C++.

Thanks,
Prathamesh

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Mike Stump]

On Aug 30, 2016, at 4:57 AM, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote:
> 
> On 30 August 2016 at 17:11, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>> On 8/29/16, Jason Merrill <jason@redhat.com> wrote:
>>> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>>>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>>>>> I tried this patch on my fork of gdb-binutils and got a few warnings
>>>>> from it. Would it be possible to have the caret point to the argument
>>>>> mentioned, instead of the function name? And also print the option
>>>>> name? E.g., instead of the current:
>>>>> 
>>>>> or32-opc.c: In function ‘or32_print_register’:
>>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>>> parameter aliases with argument 3
>>>>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>>   ^~~~~~~
>>>>> 
>>>>> could it look like:
>>>>> 
>>>>> or32-opc.c: In function ‘or32_print_register’:
>>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>>> parameter aliases with argument 3 [-Wrestrict]
>>>>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>>            ^~~~~~~~~~~~
>>>>> 
>>>>> instead?
>>>> 
>>>> I didn't try to implement it, but I think this should be fairly easy to
>>>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>>>> has arg_loc, which is a vector of parameter locations.
>>> 
>>> The C++ FE doesn't have this currently, but it could be added without
>>> too much trouble: in cp_parser_parenthesized_expression_list, extract
>>> the locations from the cp_expr return value of
>>> cp_parser_assignment_expression, and then pass the locations back up
>>> to cp_parser_postfix_expression.
>>> 
>>> Jason
>>> 
>> 
>> 
>> On the topic of how to get this warning working with various
>> frontends, is there any reason why the Objective C frontend doesn't
>> handle -Wrestrict? Currently when trying to use it, it just says:
>> 
>> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
>> but not for ObjC
> Hi Eric,
> I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
> add the option for these front-ends.
> If it is valid, I will enable the option for ObjC and Obj-C++.

This is wrong, C/C++ options should always be ObjC/ObjC++ options.

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Prathamesh Kulkarni]

On 31 August 2016 at 03:45, Mike Stump <mikestump@comcast.net> wrote:
> On Aug 30, 2016, at 4:57 AM, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote:
>>
>> On 30 August 2016 at 17:11, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>>> On 8/29/16, Jason Merrill <jason@redhat.com> wrote:
>>>> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>>>>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>>>>>> I tried this patch on my fork of gdb-binutils and got a few warnings
>>>>>> from it. Would it be possible to have the caret point to the argument
>>>>>> mentioned, instead of the function name? And also print the option
>>>>>> name? E.g., instead of the current:
>>>>>>
>>>>>> or32-opc.c: In function ‘or32_print_register’:
>>>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>>>> parameter aliases with argument 3
>>>>>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>>>   ^~~~~~~
>>>>>>
>>>>>> could it look like:
>>>>>>
>>>>>> or32-opc.c: In function ‘or32_print_register’:
>>>>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>>>>> parameter aliases with argument 3 [-Wrestrict]
>>>>>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>>>>            ^~~~~~~~~~~~
>>>>>>
>>>>>> instead?
>>>>>
>>>>> I didn't try to implement it, but I think this should be fairly easy to
>>>>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>>>>> has arg_loc, which is a vector of parameter locations.
>>>>
>>>> The C++ FE doesn't have this currently, but it could be added without
>>>> too much trouble: in cp_parser_parenthesized_expression_list, extract
>>>> the locations from the cp_expr return value of
>>>> cp_parser_assignment_expression, and then pass the locations back up
>>>> to cp_parser_postfix_expression.
>>>>
>>>> Jason
>>>>
>>>
>>>
>>> On the topic of how to get this warning working with various
>>> frontends, is there any reason why the Objective C frontend doesn't
>>> handle -Wrestrict? Currently when trying to use it, it just says:
>>>
>>> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
>>> but not for ObjC
>> Hi Eric,
>> I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
>> add the option for these front-ends.
>> If it is valid, I will enable the option for ObjC and Obj-C++.
>
> This is wrong, C/C++ options should always be ObjC/ObjC++ options.
Thanks, I will add the warning for ObjC and ObjC++.

Thanks,
Prathamesh
>