diff mbox

RFA: Fix mark_referenced_resources to handle COND_EXEC

Message ID 20130906062213.pkczc5rlwkgwwocg-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

Joern Rennecke Sept. 6, 2013, 10:22 a.m. UTC
We found that gethostbyname_r from uClibc got miscompiled for ARC because
reorg thought a COND_EXEC unconditionally kills all the conditionally set
registers.  Making it think that the registers are not killed is no good
either, because then we would miss anti-dependencies.  So, short of adding
some complex predicate-tracing across all paths, it is best to consider
the result registers to be used before set.

bootstrapped / regtested on i686-pc-linux-gnu.

Well, that's not a particular relevant port since it doesn't combine
delay_slots with COND_EXEC, but then, only ARC does, and it's still
waiting for review.  And an arc-linux-uclibc system can't boot properly
without this patch being applied to the compiler, and you have to boot
the system before doing a compiler bootstrap... and the later is also a
bit uncommon for embedded targets.

This is also the reason I placed the test into gcc.target; in principle
the test could also live in gcc.dg, but it would only be relevant to ARC.

OK to apply?
gcc:

2013-04-30  Joern Rennecke  <joern.rennecke@embecosm.com>

	* resource.c (mark_referenced_resources): Handle COND_EXEC.
gcc/testsuite:

2013-08-31  Joern Rennecke  <joern.rennecke@embecosm.com>

	* gcc.target/arc/cond-set-use.c: New test.

diff --git a/gcc/testsuite/gcc.target/arc/cond-set-use.c b/gcc/testsuite/gcc.target/arc/cond-set-use.c
new file mode 100644
index 0000000..aee2725
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arc/cond-set-use.c
@@ -0,0 +1,128 @@
+/* { dg-do run } */
+/* { dg-options "-Os" } */
+
+/* Based on gethostbyname_r,
+ * Copyright (C) 2000-2006 Erik Andersen <andersen@uclibc.org>
+ *
+ * Licensed under the LGPL v2.1, see the file COPYING.LIB
+ *
+ * Extraction / wrapping as test by
+ * Joern Rennecke  <joern.rennecke@embecosm.com>
+ * Copyright (C) 2013 Free Software Foundation, Inc.
+ */
+
+typedef unsigned size_t;
+typedef int ssize_t;
+typedef unsigned uint32_t;
+struct resolv_answer {
+ char *dotted;
+ int atype;
+ int aclass;
+ int ttl;
+ int rdlength;
+ const unsigned char *rdata;
+ int rdoffset;
+ char* buf;
+ size_t buflen;
+ size_t add_count;
+};
+struct hostent
+{
+  char *h_name;
+  char **h_aliases;
+  int h_addrtype;
+  int h_length;
+  char **h_addr_list;
+};
+
+int *__attribute__  ((noinline,weak)) nop (void * p) { return p; };
+void __attribute__  ((noinline,weak)) seta (struct resolv_answer * p)
+{ p->atype = 1;}
+
+int ghostbyname_r(
+  struct hostent *result_buf,
+  char *buf,
+  size_t buflen,
+  struct hostent **result,
+  int *h_errnop)
+{
+ char **addr_list;
+ char **alias;
+ char *alias0;
+ int i0;
+ struct resolv_answer a;
+ int i;
+
+ *result = ((void *)0);
+
+         *h_errnop = -1;
+
+         if ((ssize_t)buflen <= 5)
+  return 34;
+
+ alias = (char **)buf;
+ addr_list = (char **)buf;
+
+  /* This got turned into branch with conditional move in delay slot.  */
+ if ((ssize_t)buflen < 256)
+  return 34;
+
+
+ {
+  if (!nop(&i0)) {
+   result_buf->h_aliases = alias;
+   result_buf->h_addrtype = 2;
+   result_buf->h_length = 4;
+   result_buf->h_addr_list = addr_list;
+   *result = result_buf;
+   *h_errnop = 0;
+   return 0;
+  }
+ }
+
+
+ seta (&a);
+
+ if (a.atype == 1) {
+
+  int need_bytes = sizeof(addr_list[0]) * (a.add_count + 1 + 1);
+
+  int ips_len = a.add_count * a.rdlength;
+
+  buflen -= (need_bytes + ips_len);
+  if ((ssize_t)buflen < 0) {
+   i = 34;
+   goto free_and_ret;
+  }
+
+  result_buf->h_addrtype = 2;
+  *result = result_buf;
+  *h_errnop = 0;
+  i = 0;
+  goto free_and_ret;
+ }
+
+ /* For cse, the 1 was is loaded into a call-saved register;
+    the load was hoisted into a delay slot before the conditional load,
+    clobbering result_buf, which (conditionally) lived in the same
+    call-saved register, because mark_referenced_resources considered the
+    destination of the COND_EXEC only clobbered, but not used.  */
+ *h_errnop = 1;
+ *nop(&i0) = 1;
+ i = 2;
+
+ free_and_ret:
+ nop (&i0);
+ return i;
+}
+
+int
+main ()
+{
+  struct hostent buf, *res;
+  int i;
+  char c;
+  ghostbyname_r (&buf,  &c, 1024, &res, &i);
+  ghostbyname_r (&buf,  0, 1024, &res, &i);
+  return 0;
+}

Comments

Steven Bosscher Sept. 6, 2013, 5:14 p.m. UTC | #1
On Fri, Sep 6, 2013 at 12:22 PM, Joern Rennecke wrote:
> 2013-04-30  Joern Rennecke  <...>
>
>         * resource.c (mark_referenced_resources): Handle COND_EXEC.

This is OK.

Ciao!
Steven
diff mbox

Patch

Index: resource.c
===================================================================
--- resource.c	(revision 202295)
+++ resource.c	(working copy)
@@ -374,6 +374,16 @@  mark_referenced_resources (rtx x, struct
     case INSN:
     case JUMP_INSN:
 
+      if (GET_CODE (PATTERN (x)) == COND_EXEC)
+      /* In addition to the usual references, also consider all outputs
+	 as referenced, to compensate for mark_set_resources treating
+	 them as killed.  This is similar to ZERO_EXTRACT / STRICT_LOW_PART
+	 handling, execpt that we got a partial incidence instead of a partial
+	 width.  */
+      mark_set_resources (x, res, 0,
+			  include_delayed_effects
+			  ? MARK_SRC_DEST_CALL : MARK_SRC_DEST);
+
 #ifdef INSN_REFERENCES_ARE_DELAYED
       if (! include_delayed_effects
 	  && INSN_REFERENCES_ARE_DELAYED (x))