diff mbox

PowerPC section type conflict (created PR 51623)

Message ID 20111228173957.GA23383@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner Dec. 28, 2011, 5:39 p.m. UTC 
On Mon, Dec 19, 2011 at 11:45:35PM +0800, Chung-Lin Tang wrote:
> On 2011/12/19 上午 03:18, Richard Henderson wrote:
> > On 12/17/2011 10:36 PM, Chung-Lin Tang wrote:
> >> I don't think it's that kind of problem; the powerpc backend uses
> >> unlikely_text_section_p(), which compares the passed in argument section
> >> and the value of function_section_1(current_function_decl,true).
> > 
> > I think this might be the real bug, or something related.
> > 
> >> Since current_function_decl is NULL at assembly phase, it retrieves
> >> ".text.unlikely" to test for equality. It's the retrieving/lookup that
> >> fails here, because the default looked-up section flags set when decl ==
> >> NULL does not really seem to make sense (adds SECTION_WRITE).
> > 
> > current_function_decl is only null when we're not inside a function.
> > 
> > One possible fix is to test for current_function_section inside
> > unlikely_text_section_p.  However, I think that begs the question
> > of what in the world is actually going on in rs6000_assemble_integer.
> > Why are we testing for emitting data in text sections?
> 
> I think I sort of mis-represented the context here; this was not really
> during the assembly phase of a function, but already in
> toplev.c:output_object_blocks().
> 
> I've created a bugzilla PR for this, with a testcase from U-boot, and a
> minimal testcase: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51623

This one line patch fixes the problem by using a different test than
unlikely_text_section_p, which assumes it is called within a function context.

I bootstrapped it, and there were no regressions.  I have added the test case
from the PR so it doesn't come back.  Is it ok to apply?  It is also a bug in
GCC 4.6, and I will backport the patch to that branch as well.

FWIW, I wrote -mrelocatable around 1990 or so to for a specific Cygnus customer
that needed to have pseudo shared libraries in embedded code, as long as they
were willing to live with various restrictions.  At the time, the Linux shared
library code was non-existant, and this was a quick hack.  In the nature of all
quick hacks, eventually things change in the machine independent code layer,
and it has to be revisited.  In hindsight, it would have been better if the
Linux shared library code was operational, and that there was a non-GPL dynamic
linker written to handle the relocations, rather than having this quick hack.

The check for unlikely text was added in 2004 by Caroline Tice of Apple, and it
is curious that they didn't add a check for it being in a hot function as well
as a cold function.  I also suspect the check would not work as well if
-ffunctions-section was used.  The point of the check is not to add to the
fixup table pointers that are stored in the read-only text section (which would
cause a segfault at runtime, but it would leave a pointer that is not fixed up
when the program starts).  It was modified in 2005 by Richard Sandiford in a
global change in how sections are dealt with, and modified by Alan Modra in
2006.

[gcc]
2011-12-27  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/51623
	* config/rs6000/rs6000.c (rs6000_assemble_integer): Don't call
	unlikely_text_section_p.  Instead check for being in a code
	section.

[gcc/testsuite]
2011-12-27  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/51623
	* gcc.target/powerpc/pr51623.c: New file.

Comments

Richard Henderson Dec. 28, 2011, 8:34 p.m. UTC | #1 
On 12/28/2011 09:39 AM, Michael Meissner wrote:
>  	  && in_section != text_section
> -	  && !unlikely_text_section_p (in_section)
> +	  && (in_section && (in_section->common.flags & SECTION_CODE)) == 0

You should be able to delete the text_section test as well,
and in_section should *never* be null, when emitting data.

Otherwise this looks much better to me.


r~
Michael Meissner Dec. 28, 2011, 8:42 p.m. UTC | #2 
On Wed, Dec 28, 2011 at 12:34:25PM -0800, Richard Henderson wrote:
> On 12/28/2011 09:39 AM, Michael Meissner wrote:
> >  	  && in_section != text_section
> > -	  && !unlikely_text_section_p (in_section)
> > +	  && (in_section && (in_section->common.flags & SECTION_CODE)) == 0
> 
> You should be able to delete the text_section test as well,
> and in_section should *never* be null, when emitting data.
> 
> Otherwise this looks much better to me.

Yeah, I thought about that.  I'm wondering whether any integer is ever emitted
in the text section, and just delete the two lines.  I'll try it out.
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 182694)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -15461,7 +15461,7 @@  rs6000_assemble_integer (rtx x, unsigned
       if (TARGET_RELOCATABLE
 	  && in_section != toc_section
 	  && in_section != text_section
-	  && !unlikely_text_section_p (in_section)
+	  && (in_section && (in_section->common.flags & SECTION_CODE)) == 0
 	  && !recurse
 	  && GET_CODE (x) != CONST_INT
 	  && GET_CODE (x) != CONST_DOUBLE
Index: gcc/testsuite/gcc.target/powerpc/pr51623.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr51623.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr51623.c	(revision 0)
@@ -0,0 +1,123 @@ 
+/* PR target/51623 */
+/* { dg-do compile { target { { powerpc*-*-linux* && ilp32 } || { powerpc-*-eabi* } } } } */
+/* { dg-options "-mrelocatable -ffreestanding" } */
+
+/* This generated an error, since the compiler was calling
+   unlikely_text_section_p in a context where it wasn't valid.  */
+
+typedef long long loff_t;
+typedef unsigned size_t;
+
+
+struct mtd_info {
+  unsigned writesize;
+  unsigned oobsize;
+  const char *name;
+};
+
+extern int strcmp(const char *,const char *);
+extern char * strchr(const char *,int);
+
+struct cmd_tbl_s {
+  char *name;
+};
+
+
+int printf(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2)));
+void* malloc(size_t);
+void free(void*);
+
+
+extern int nand_curr_device;
+extern struct mtd_info nand_info[];
+
+static int nand_dump(struct mtd_info *nand, unsigned long off, int only_oob)
+{
+  int i;
+  unsigned char *datbuf, *oobbuf, *p;
+
+  datbuf = malloc(nand->writesize + nand->oobsize);
+  oobbuf = malloc(nand->oobsize);
+  off &= ~(nand->writesize - 1);
+
+  printf("Page %08lx dump:\n", off);
+  i = nand->writesize >> 4;
+  p = datbuf;
+
+  while (i--) {
+    if (!only_oob)
+      printf("\t%02x %02x %02x %02x %02x %02x %02x %02x"
+	     "  %02x %02x %02x %02x %02x %02x %02x %02x\n",
+	     p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7],
+	     p[8], p[9], p[10], p[11], p[12], p[13], p[14],
+	     p[15]);
+    p += 16;
+  }
+
+  i = nand->oobsize >> 3;
+  free(datbuf);
+  free(oobbuf);
+
+  return 0;
+}
+
+int do_nand(struct cmd_tbl_s * cmdtp, int flag, int argc, char *argv[])
+{
+  int dev;
+  unsigned long  off;
+  char *cmd, *s;
+  struct mtd_info *nand;
+
+  if (argc < 2)
+    goto usage;
+
+  cmd = argv[1];
+
+  if (strcmp(cmd, "info") == 0) {
+    putc('\n');
+    return 0;
+  }
+
+  if (strcmp(cmd, "device") == 0) {
+    if (argc < 3) {
+      putc('\n');
+    }
+    dev = (int)simple_strtoul(argv[2], ((void *)0), 10);
+    nand_curr_device = dev;
+    return 0;
+  }
+
+  if (strcmp(cmd, "bad") != 0 && strcmp(cmd, "erase") != 0  )
+    goto usage;
+  
+  if (nand_curr_device < 0 ) {
+    return 1;
+  }
+  nand = &nand_info[nand_curr_device];
+
+  if (strcmp(cmd, "erase") == 0 || strcmp(cmd, "scrub") == 0) {
+    int clean = argc > 2 && !strcmp("clean", argv[2]);
+    int scrub = !strcmp(cmd, "scrub");
+    return 0;
+  }
+
+  if (strncmp(cmd, "dump", 4) == 0) {
+    if (argc < 3)
+      goto usage;
+
+    s = strchr(cmd, '.');
+    off = (int)simple_strtoul(argv[2], ((void *)0), 16);
+    
+    if (s != ((void *)0) && strcmp(s, ".oob") == 0)
+      nand_dump(nand, off, 1);
+    else
+      nand_dump(nand, off, 0);
+    
+    return 0;
+  }
+usage:
+  cmd_usage(cmdtp);
+  return 1;
+}
+
+void *ptr = do_nand;