On 10/15/2021 4:39 AM, Aldy Hernandez wrote:
On 10/15/21 2:47 AM, Andrew MacLeod wrote:
On 10/14/21 6:07 PM, Martin Sebor via Gcc-patches wrote:
On 10/9/21 12:47 PM, Aldy Hernandez via Gcc-patches wrote:
We seem to be passing a lot of context around in the strlen code. I
certainly don't want to contribute to more.
Most of the handle_* functions are passing the gsi as well as either
ptr_qry or rvals. That looks a bit messy. May I suggest putting all
of that in the strlen pass object (well, the dom walker object, but we
can rename it to be less dom centric)?
Something like the attached (untested) patch could be the basis for
further cleanups.
Jakub, would this line of work interest you?
You didn't ask me but since no one spoke up against it let me add
some encouragement: this is exactly what I was envisioning and in
line with other such modernization we have been doing elsewhere.
Could you please submit it for review?
Martin
I'm willing to bet he didn't submit it for review because he doesn't
have time this release to polish and track it... (I think the
threader has been quite consuming). Rather, it was offered as a
starting point for someone else who might be interested in continuing
to pursue this work... *everyone* is interested in cleanup work
others do :-)
Exactly. There's a lot of work that could be done in this area, and
I'm trying to avoid the situation with the threaders where what
started as refactoring ended up with me basically owning them ;-).
That being said, I there are enough cleanups that are useful on their
own. I've removed all the passing around of GSIs, as well as ptr_qry,
with the exception of anything dealing with the sprintf pass, since it
has a slightly different interface.
This is patch 0001, which I'm formally submitting for inclusion. No
functional changes with this patch. OK for trunk?
Also, I am PINGing patch 0002, which is the strlen pass conversion to
the ranger. As mentioned, this is just a change from an evrp client
to a ranger client. The APIs are exactly the same, and besides, the
evrp analyzer is deprecated and slated for removal. OK for trunk?
Aldy
0001-Convert-strlen-pass-from-evrp-to-ranger.patch
From 152bc3a1dad9a960b7c0c53c65d6690532d9da5a Mon Sep 17 00:00:00 2001
From: Aldy Hernandez<al...@redhat.com>
Date: Fri, 8 Oct 2021 15:54:23 +0200
Subject: [PATCH] Convert strlen pass from evrp to ranger.
The following patch converts the strlen pass from evrp to ranger,
leaving DOM as the last remaining user.
No additional cleanups have been done. For example, the strlen pass
still has uses of VR_ANTI_RANGE, and the sprintf still passes around
pairs of integers instead of using a proper range. Fixing this
could further improve these passes.
Basically the entire patch is just adjusting the calls to range_of_expr
to include context. The previous context of si->stmt was mostly
empty, so not really useful ;-).
With ranger we are now able to remove the range calculation from
before_dom_children entirely. Just working with the ranger on-demand
catches all the strlen and sprintf testcases with the exception of
builtin-sprintf-warn-22.c which is due to a limitation of the sprintf
code. I have XFAILed the test and documented what the problem is.
On a positive note, these changes found two possible sprintf overflow
bugs in the C++ and Fortran front-ends which I have fixed below.
Tested on x86-64 Linux.
gcc/ChangeLog:
* tree-ssa-strlen.c (compare_nonzero_chars): Pass statement
context to ranger.
(get_addr_stridx): Same.
(get_stridx): Same.
(get_range_strlen_dynamic): Same.
(handle_builtin_strlen): Same.
(handle_builtin_strchr): Same.
(handle_builtin_strcpy): Same.
(maybe_diag_stxncpy_trunc): Same.
(handle_builtin_stxncpy_strncat):
(handle_builtin_memcpy): Same.
(handle_builtin_strcat): Same.
(handle_alloc_call): Same.
(handle_builtin_memset): Same.
(handle_builtin_string_cmp): Same.
(handle_pointer_plus): Same.
(count_nonzero_bytes_addr): Same.
(count_nonzero_bytes): Same.
(handle_store): Same.
(fold_strstr_to_strncmp): Same.
(handle_integral_assign): Same.
(check_and_optimize_stmt): Same.
(class strlen_dom_walker): Replace evrp with ranger.
(strlen_dom_walker::before_dom_children): Remove evrp.
(strlen_dom_walker::after_dom_children): Remove evrp.
* gimple-ssa-warn-access.cc (maybe_check_access_sizes):
Restrict sprintf output.
gcc/cp/ChangeLog:
* ptree.c (cxx_print_xnode): Add more space to pfx array.
gcc/fortran/ChangeLog:
* misc.c (gfc_dummy_typename): Make sure ts->kind is
non-negative.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/builtin-sprintf-warn-22.c: XFAIL.
OK. Had I realized 99% was just adding the new argument to a bunch of
call sites, I would have taken care of it earlier.
jeff
