Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Jeff Law <law at redhat dot com>]

Ignore this prior message from me....  I'm off-base here...


On 08/15/2018 12:13 PM, Jeff Law wrote:
> On 08/15/2018 10:38 AM, Bernd Edlinger wrote:
>> On 08/15/18 18:25, Jeff Law wrote:
>>> On 08/14/2018 04:25 AM, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> this patch is a follow-up to my patch here:
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html
>>>>
>>>> Since most calls of c_strlen and get_range_strlen expect
>>>> a string length in bytes of a zero-terminated string, there is
>>>> a need for a new parameter eltsize, which is per default 1,
>>>> but can be used in gimple-ssa-sprintf.c to specify the
>>>> expected character size.
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>
>>>> patch-fix-c-strlen.diff
>>>>
>>>>
>>>> 2018-08-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* builtins.c (c_strlen): Add new parameter eltsize.
>>>> 	* builtins.h (c_strlen): Adjust prototype.
>>>> 	* expr.c (string_constant): Add new parameter mem_size.
>>>> 	* expr.h (string_constant): Adjust protoype.
>>>> 	* gimple-fold.c (get_range_strlen): Add new parameter eltsize.
>>>> 	* gimple-fold.h (get_range_strlen): Adjust prototype.
>>>> 	* gimple-ssa-sprintf.c (get_string_length): Add new parameter eltsize.
>>>> 	(format_string): Call get_string_length with eltsize.
>>>>
>>>> 2018-08-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* gcc.dg/strlenopt-49.c: Adjust test case.
>>>> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-18.c: Likewise.
>>>
>>> Your patch appears to pass in an ELTSIZE which is used to determine how
>>> to count.  This has some advantages in that the routine can be used to
>>> count bytes or other elements such as 2 or 4 byte wide characters.
>>>
>>> Your testsuite change appears to remove the xfails in strlenopt-49 which
>>> is usually OK.  So no concerns there.
>>>
>>> The other testsuite change shows that the issued warning has changed.
>>> But we're still issuing a diagnostic the the construct, so it looks
>>> reasonable to me.
>>>
>>>
>>> This seems conceptually independent from the rest of the stuff we need
>>> to work through.  Though there definitely a potential for
>>> implementation/merge conflicts.
>>>
>>> My concern is the sprintf changes -- for the wide char case we'll be
>>> getting a count of wide chars in the string.  I'm not offhand sure if
>>> the rest of the sprintf warning bits are prepared to count that way.
>>>
>>> Martin?
>>>
>>
>> I think the %S issue is an independet thing, I have seen
>> a patch where %S sets the FMT_LEN_l, so I left that one alone.
>>
>> What I have no solution for, is which wide char-type to assume.
>> I used 4, but that is only correct for some O/S. Others have 2.
> I wasn't very clear.  Sorry.
> 
> My concern is that with your change you're asking format_string to count
> in wide character chunks for FMT_LEN_l.  Yet the rest of the sprintf
> pass counts in bytes.  See for example the computation of the
> destination buffer size.  Your change would introduce the exact kind of
> inconsistency it's supposed to avoid.  In fact everything which relies
> on the underlying builtin_object_size infrastructure counts in bytes.
> 
> AFAICT that's the only place where you pass anything other than byte
> requests to c_strlen.  So unless you have other places where you expect
> to be passing in requests to count by 2 or 4 bytes it seems this patch
> is unnecessary and dangerous.
> 
> Am I missing something?
> 
> jeff
>