waccess: Remove visited bitmap and stop on EDGE_ABNORMAL

Message ID YiMabZ98KYj+MCuR@tucnak
State New
Headers
Series waccess: Remove visited bitmap and stop on EDGE_ABNORMAL |

Commit Message

Jakub Jelinek March 5, 2022, 8:08 a.m. UTC
  On Fri, Mar 04, 2022 at 02:58:37PM +0100, Jakub Jelinek via Gcc-patches wrote:
> On Thu, Mar 03, 2022 at 05:08:30PM -0700, Martin Sebor wrote:
> > > 1) shouldn't it give up for EDGE_ABNORMAL too?  I mean, e.g.
> > >     following a non-local goto forced edge from a noreturn call
> > >     to a non-local label (if there is just one) doesn't seem
> > >     right to me
> > 
> > Possibly yes.  I can add it but I don't have a lot of experience with
> > these bits so if you can suggest a test case to exercise this that
> > would be helpful.
> 
> Something like:
> void
> foo (void)
> {
>   __label__ l;
>   __attribute__((noreturn)) void bar (int x) { if (x) goto l; __builtin_trap (); }
>   bar (0);
> l:;
> }
> shows a single EDGE_ABNORMAL from the bar call.
> But it would need tweaking for the ptr use and clobber.
> 
> > > 2) if EDGE_DFS_BACK is computed and 1) is done, is there any
> > >     reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK
> > >     check as well as the visited bitmap (and having them use
> > >     very different answers, if EDGE_DFS_BACK is seen, the function
> > >     will return false, if visited bitmap has a bb, it will return true)?
> > >     Can't the visited bitmap go away?
> > 
> > Possibly.  As I said above, I don't have enough experience with these
> > bits to make (and test) the changes quickly, or enough bandwidth to
> > come up to speed on them.  Please feel free to make these improvements.
> 
> I'll change that if it passes testing.

Here is a patch to do both.  I don't think we really need to have a testcase
for the EDGE_ABNORMAL case (Martin, feel free to add it later), abnormal
edges simply aren't normal control flow and what exactly it means varies.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-03-05  Jakub Jelinek  <jakub@redhat.com>

	* gimple-ssa-warn-access.cc (pass_waccess::use_after_inval_p): Remove
	visited bitmap and its use.  Also punt on EDGE_ABNORMAL edges.



	Jakub
  

Comments

Richard Biener March 5, 2022, 10:28 a.m. UTC | #1
> Am 05.03.2022 um 09:08 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> On Fri, Mar 04, 2022 at 02:58:37PM +0100, Jakub Jelinek via Gcc-patches wrote:
>> On Thu, Mar 03, 2022 at 05:08:30PM -0700, Martin Sebor wrote:
>>>> 1) shouldn't it give up for EDGE_ABNORMAL too?  I mean, e.g.
>>>>    following a non-local goto forced edge from a noreturn call
>>>>    to a non-local label (if there is just one) doesn't seem
>>>>    right to me
>>> 
>>> Possibly yes.  I can add it but I don't have a lot of experience with
>>> these bits so if you can suggest a test case to exercise this that
>>> would be helpful.
>> 
>> Something like:
>> void
>> foo (void)
>> {
>>  __label__ l;
>>  __attribute__((noreturn)) void bar (int x) { if (x) goto l; __builtin_trap (); }
>>  bar (0);
>> l:;
>> }
>> shows a single EDGE_ABNORMAL from the bar call.
>> But it would need tweaking for the ptr use and clobber.
>> 
>>>> 2) if EDGE_DFS_BACK is computed and 1) is done, is there any
>>>>    reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK
>>>>    check as well as the visited bitmap (and having them use
>>>>    very different answers, if EDGE_DFS_BACK is seen, the function
>>>>    will return false, if visited bitmap has a bb, it will return true)?
>>>>    Can't the visited bitmap go away?
>>> 
>>> Possibly.  As I said above, I don't have enough experience with these
>>> bits to make (and test) the changes quickly, or enough bandwidth to
>>> come up to speed on them.  Please feel free to make these improvements.
>> 
>> I'll change that if it passes testing.
> 
> Here is a patch to do both.  I don't think we really need to have a testcase
> for the EDGE_ABNORMAL case (Martin, feel free to add it later), abnormal
> edges simply aren't normal control flow and what exactly it means varies.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard 

> 2022-03-05  Jakub Jelinek  <jakub@redhat.com>
> 
>    * gimple-ssa-warn-access.cc (pass_waccess::use_after_inval_p): Remove
>    visited bitmap and its use.  Also punt on EDGE_ABNORMAL edges.
> 
> --- gcc/gimple-ssa-warn-access.cc.jj    2022-03-03 22:09:22.073800776 +0100
> +++ gcc/gimple-ssa-warn-access.cc    2022-03-04 19:21:18.840416075 +0100
> @@ -3812,20 +3812,15 @@ pass_waccess::use_after_inval_p (gimple
>       /* Proceed only when looking for uses of dangling pointers.  */
>       auto gsi = gsi_for_stmt (use_stmt);
> 
> -      auto_bitmap visited;
> -
>       /* A use statement in the last basic block in a function or one that
>     falls through to it is after any other prior clobber of the used
>     variable unless it's followed by a clobber of the same variable. */
>       basic_block bb = use_bb;
>       while (bb != inval_bb
>         && single_succ_p (bb)
> -         && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK)))
> +         && !(single_succ_edge (bb)->flags
> +          & (EDGE_EH | EDGE_ABNORMAL | EDGE_DFS_BACK)))
>    {
> -      if (!bitmap_set_bit (visited, bb->index))
> -        /* Avoid cycles. */
> -        return true;
> -
>      for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
>        {
>          gimple *stmt = gsi_stmt (gsi);
> 
> 
>    Jakub
>
  

Patch

--- gcc/gimple-ssa-warn-access.cc.jj	2022-03-03 22:09:22.073800776 +0100
+++ gcc/gimple-ssa-warn-access.cc	2022-03-04 19:21:18.840416075 +0100
@@ -3812,20 +3812,15 @@  pass_waccess::use_after_inval_p (gimple
       /* Proceed only when looking for uses of dangling pointers.  */
       auto gsi = gsi_for_stmt (use_stmt);
 
-      auto_bitmap visited;
-
       /* A use statement in the last basic block in a function or one that
 	 falls through to it is after any other prior clobber of the used
 	 variable unless it's followed by a clobber of the same variable. */
       basic_block bb = use_bb;
       while (bb != inval_bb
 	     && single_succ_p (bb)
-	     && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK)))
+	     && !(single_succ_edge (bb)->flags
+		  & (EDGE_EH | EDGE_ABNORMAL | EDGE_DFS_BACK)))
 	{
-	  if (!bitmap_set_bit (visited, bb->index))
-	    /* Avoid cycles. */
-	    return true;
-
 	  for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
 	    {
 	      gimple *stmt = gsi_stmt (gsi);