This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 08/15/2018 12:51 PM, Bernd Edlinger wrote:
[ snip -- comment attribution is likely lost... ]

>>>>
>>>> 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.
[ ... ]
So yesterday I sent a message where I was totally offbase with the
intent of this patch.  After further discussions and re-review ISTM the
real intent here is to restore prior behavior of counting bytes to the
clients outside the sprintf pass.  Terribly sorry for that and the
conclusions I drew as a result.




>>
>> 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.
No bugs that we're currently aware of. But ensuring that routines that
expect to be counting bytes are counting bytes is a good thing.  If some
pass (say forwprop) changed the char * argument to an array of something
other than chars then we'd likely end up giving a wrong result.

I don't mind making the interface narrower with a boolean or an enum to
specify how we count rather than having the caller pass in a type.


>>
>>> Your testsuite change appears to remove the xfails in strlenopt-49 which
>>> is usually OK.  So no concerns there.
So it turns out we don't really need any of the testsuite changes anyway
if we make this patch independent of the 86711/86714 patches.  And
breaking that dependency is IMHO a good thing.

[ Another snip. ]

> 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);
Presumably  "||" is missing here.

> 
> 
> 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.
If we make the patch to add the size parameter to c_strlen independent
of the changes related to 86711/86714, then we don't need to xfail or
change the testsuite in any way shape or form.

My overall thinking is to carve out this patch so that it's independent
of anything else.  That's actually easy to do after addressing two very
minor issues.

We can then add Martin's patch to fix handling of wide chars in sprintf
(86853).

--

This overall plan addresses the need to be able to tell c_strlen how to
count and reverts its behavior for callers outside of the sprintf
warning pass to historical (pre-gcc7) behavior of counting bytes by
default.  It does this without losing any warnings and allow us to move
forward with Martin's more complete fix for 86853.

With those two patches out of the way we can then dig further into the
issues around 86711/86714.


jeff




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]