This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen
On 08/15/18 20:18, Martin Sebor wrote:
> On 08/15/2018 10:25 AM, 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.
>
> Parameterizing the function to return either the number of
> bytes or the number of elements makes sense as an enhancement.
> It makes less sense (and could be the source of bugs) to let
> callers pass in anything else. A boolean flag to toggle
> between the two alternatives would make the API narrower
> and safer to use. It doesn't seem necessary to fix any
> bugs.
>
>> Your testsuite change appears to remove the xfails in strlenopt-49 which
>> is usually OK. So no concerns there.
>
> The change doesn't restore all the tests. It leaves in place
> the last xfail (added by the prior patch in this series, and
> not on trunk):
>
> { dg-final { scan-tree-dump-times "cmp88 = 0;" 1 "gimple" { xfail *-*-* } } }
>
> The test case verifies that the memcmp call below is folded:
>
> const char a8[8] = "1234567\0";
>
> n = memcmp (a8, "1234567\0", sizeof a8);
>
> The call is safe and well-defined and there is no reason not
> to fold it.
>
>> 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?
>
> The sprintf changes (both the tests and the code) overlap with
> the fix I posted for PR 86853.
>
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00345.html
>
> The text of the new diagnostic ("writing a terminating nul past
> the end of the destination") also implies that the changes
> aren't correct/complete: the %S directive itself should be
> diagnosed (not just the whole call) and the expected text is
> "writing up to 6 bytes" (for up to MB_LEN_MAX).
>
> It's almost impossible for me to make progress with this incessant
> interference. It makes no sense to have two people doing the same
> work at the same time.
>
Sorry, about that.
Anyway, I thought your patch would convert dir.specifier == 'S'
into dir.modifier == FMT_LEN_l, since %S is like %ls.
the xfail will go away if I change this:
get_string_length (arg, dir.modifier == FMT_LEN_l ? 4 : 1);
to:
get_string_length (arg, dir.specifier == 'S' dir.modifier == FMT_LEN_l ? 4 : 1);
That should be easy, I will change the patch to match your patch in that
area.
The other xfail with memcmp needs to be fixed separately.
The issue is, callers of string_constant that do not pass the mem_size
parameter may access up to TREE_STRING_LENGTH byte, therefore
returning longer strings is not appropriate: the string constant
contains 9 bytes, mind the terminating nul.
Bernd.