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.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. >
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.
