public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Georg-Johann Lay <avr@gjlay.de>
To: gcc-patches@gcc.gnu.org
Cc: ebotcazou@libertysurf.fr
Subject: [patch] Fix PR101188 wrong code from postreload
Date: Fri, 2 Jun 2023 10:46:41 +0200	[thread overview]
Message-ID: <55462f69-84b8-6efb-b98e-399390928420@gjlay.de> (raw)

There is the following bug in postreload that can be traced back
to v5 at least:

In postreload.cc::reload_cse_move2add() there is a loop over all
insns.  If it encounters a SET, the next insn is analyzed if it
is a single_set.

After next has been analyzed, it continues with

       if (success)
	delete_insn (insn);
       changed |= success;
       insn = next; // This effectively skips analysis of next.
       move2add_record_mode (reg);
       reg_offset[regno]
	= trunc_int_for_mode (added_offset + base_offset,
			      mode);
       continue; // for continues with insn = NEXT_INSN (insn).

So it records the effect of next, but not the clobbers that
next might have.  This is a problem if next clobbers a GPR
like it can happen for avr.  What then can happen is that in a
later round, it may use a value from a (partially) clobbered reg.

The patch records the effects of potential clobbers.

Bootstrapped and reg-tested on x86_64.  Also tested on avr where
the bug popped up.  The testcase discriminates on avr, and for now
I am not aware of any other target that's affected by the bug.

The change is not intrusive and fixes wrong code, so I'd like
to backport it.

Ok to apply?

Johann

rtl-optimization/101188: Don't bypass clobbers of some insns that are
optimized or are optimization candidates.

gcc/
	PR rtl-optimization/101188
	* postreload.cc (reload_cse_move2add): Record clobbers of next
	insn using move2add_note_store.

gcc/testsuite/
	PR rtl-optimization/101188
	* gcc.c-torture/execute/pr101188.c: New test.


diff --git a/gcc/postreload.cc b/gcc/postreload.cc
index fb392651e1b..2de3e2ea780 100644
--- a/gcc/postreload.cc
+++ b/gcc/postreload.cc
@@ -2033,6 +2033,14 @@ reload_cse_move2add (rtx_insn *first)
  		      if (success)
  			delete_insn (insn);
  		      changed |= success;
+		      // By setting "insn = next" below, we are bypassing the
+		      // side-effects of next, see PR101188.  Do them by hand
+		      subrtx_iterator::array_type array;
+		      FOR_EACH_SUBRTX (iter, array, PATTERN (next), NONCONST)
+			{
+			  if (GET_CODE (*iter) == CLOBBER)
+			    move2add_note_store (XEXP (*iter, 0), *iter, next);
+			}
  		      insn = next;
  		      move2add_record_mode (reg);
  		      reg_offset[regno]
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr101188.c 
b/gcc/testsuite/gcc.c-torture/execute/pr101188.c
new file mode 100644
index 00000000000..4817c69347c
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr101188.c
@@ -0,0 +1,59 @@
+typedef __UINT8_TYPE__ uint8_t;
+typedef __UINT16_TYPE__ uint16_t;
+
+typedef uint8_t (*fn1)(void *a);
+typedef void (*fn2)(void *a, int *arg);
+
+struct S
+{
+    uint8_t buffer[64];
+    uint16_t n;
+    fn2 f2;
+    void *a;
+    fn1 f1;
+};
+
+volatile uint16_t x;
+
+void __attribute__((__noinline__,__noclone__))
+foo (uint16_t n)
+{
+  x = n;
+}
+
+void __attribute__((__noinline__,__noclone__))
+testfn (struct S *self)
+{
+    int arg;
+
+    foo (self->n);
+    self->n++;
+    self->f2 (self->a, &arg);
+    self->buffer[0] = self->f1 (self->a);
+}
+
+static unsigned char myfn2_called = 0;
+
+static void
+myfn2 (void *a, int *arg)
+{
+  myfn2_called = 1;
+}
+
+static uint8_t
+myfn1 (void *a)
+{
+  return 0;
+}
+
+int main (void)
+{
+  struct S s;
+  s.n = 0;
+  s.f2 = myfn2;
+  s.f1 = myfn1;
+  testfn (&s);
+  if (myfn2_called != 1)
+    __builtin_abort();
+  return 0;
+}

             reply	other threads:[~2023-06-02  8:46 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-02  8:46 Georg-Johann Lay [this message]
2023-06-03 15:53 ` Jeff Law
2023-06-03 18:38   ` Georg-Johann Lay
2023-06-10 16:45     ` Jeff Law
2023-06-05 15:06   ` Georg-Johann Lay
2023-06-05 15:55     ` Jeff Law

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=55462f69-84b8-6efb-b98e-399390928420@gjlay.de \
    --to=avr@gjlay.de \
    --cc=ebotcazou@libertysurf.fr \
    --cc=gcc-patches@gcc.gnu.org \
    /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).