public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jeff Law <jeffreyalaw@gmail.com>
To: Richard Biener <rguenther@suse.de>
Cc: gcc-patches@gcc.gnu.org, Jan Hubicka <hubicka@ucw.cz>
Subject: Re: [PATCH] Improve DSE to handle stores before __builtin_unreachable ()
Date: Wed, 21 Jun 2023 08:04:13 -0600	[thread overview]
Message-ID: <b976b836-e2ef-7b6c-05b8-db1e50752811@gmail.com> (raw)
In-Reply-To: <nycvar.YFH.7.77.849.2306210620560.4723@jbgna.fhfr.qr>



On 6/21/23 00:41, Richard Biener wrote:
>> I thought during the introduction of erroneous path isolation that we
>> concluded stores, calls and such had observable side effects that must be
>> preserved, even when we hit a block that leads to __builtin_unreachable.
> 
> Indeed, I remember we repeatedly hit this in the past.  But
> double-checking I see that we instrument
> 
>    if (x)
>      *(int *)0 = 0;
> 
> as
> 
>    <bb 2> [local count: 1073741824]:
>    if (x_2(D) != 0)
>      goto <bb 3>; [50.00%]
>    else
>      goto <bb 4>; [50.00%]
> 
>    <bb 3> [local count: 536870913]:
>    MEM[(int *)0B] ={v} 0;
>    __builtin_trap ();
> 
> path isolation doesn't seem to use __builtin_unreachable ().  I did
> not add __builtin_trap () as possible sink (but I did want to treat
> __builtin_unreachable () and __builtin_unreachable_trap () the same
> way).  The pass also marks the offending store as volatile.
Nuts.  I mixed up trap vs unreachable in my own head.  Though I think 
for the purposes of this issue they should be treated the same.  The 
only difference is one actively halts the code, the other silently lets 
it keep going.


> So yes, I think preserving the original trap kind (if there is any)
> is important and it still seems to work.  I don't remember whether
> we have any test coverage for that though.  I'll also note that
> __builtin_trap () has virtual operands (def and use) while
> __builtin_unreachable[_trap] () are 'const'.  Honza correctly
> says they should probably be novops instead of 'const' preserving
> the fact that they have side-effects.
If we have test coverage it's probably minimal -- a few things to 
validate proper behavior around builtin_trap plus whatever regressions 
came up.


> I think it's desirable for assertions.  Since we elide plain
> __builtin_unreachable () and fall thru whereever it leads that
> shouldn't be an issue.
> 
> If I manually add a __builtin_unreachable () to the above case
> I see the *(int *)0 = 0; store DSEd.  Maybe we should avoid
> removing stores that might trap here?  POSIX wise such a trap
> could be a way to jump out of the path leading to unreachable ()
> via siglongjmp ...
Yea, removing the store seemswrong .  As you note, someone could have a 
handler to catch the store, then longjump elsewhere to continue some 
kind of sensible execution.

The erroneous path isolation bits have some code to try and clean up the 
bogus path a bit.  Ideally leaving a single statement with undefined 
beahvior and the trap.  I wonder if there's any code you could re-use 
from that effort.

Essentially a mini pass that cleans up paths post-dominated by a 
builtin_unreachable or builtin_trap.



jeff


  parent reply	other threads:[~2023-06-21 14:04 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20230620070009.C11983858D1E@sourceware.org>
2023-06-20 13:27 ` Jeff Law
2023-06-21  6:41   ` Richard Biener
2023-06-21  9:55     ` Jan Hubicka
2023-06-21 14:04     ` Jeff Law [this message]
2023-06-22  6:31       ` Richard Biener
2023-06-22 13:36         ` Jeff Law
2023-06-22 13:42           ` Jan Hubicka
2023-06-24 14:24             ` Jeff Law
2023-06-25 16:33               ` Jan Hubicka
2023-06-26 17:21   ` Jan Hubicka
2023-06-26 22:37     ` Jeff Law
2023-06-20  6:59 Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b976b836-e2ef-7b6c-05b8-db1e50752811@gmail.com \
    --to=jeffreyalaw@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hubicka@ucw.cz \
    --cc=rguenther@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).