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.edlin...@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.edlin...@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.
Martin
