[PATCH] PowerPC section type conflict

[PATCH] PowerPC section type conflict

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]


Hi, under powerpc targets, using -mrelocatable under some cases triggers
an error during final assembly generation: rs6000_assemble_integer()
calls varasm.c:unlikely_text_section_p(), and the call chain eventually
gets to where the section htab is queried for ".text.unlikely", and
fails because of mismatched section flags: "error: foo causes a section
type conflict"

This seems to happen after rev.167085, 4.6 branch is also affected.

The flag mismatch seems quite straightforward: current_function_decl is
passed in as the decl here, and as it's final assembly now, it is
NULL_TREE. varasm.c:default_section_type_flags() adds SECTION_WRITE to
its return value, and things get borked.

This patch simply adds a condition to the SECTION_CODE case, to include
when decl == NULL_TREE, and the queried section has a .text* name. I
think this should be the intuitive way, and it does allow the testcase
to compile.

Tested on a powerpc target, is this okay for trunk? (and maybe 4.6 too?)

Thanks,
Chung-Lin

2011-12-16  Chung-Lin Tang  <cltang@codesourcery.com>

	gcc/
	* varasm.c (default_section_type_flags): Set SECTION_CODE in
	flags when decl is NULL_TREE, and section name matches ".text*".


[-- Attachment #2: varasm.diff --]
[-- Type: text/plain, Size: 415 bytes --]

Index: trunk/gcc/varasm.c
===================================================================
--- trunk/gcc/varasm.c	(revision 182398)
+++ trunk/gcc/varasm.c	(working copy)
@@ -6218,7 +6218,8 @@
 {
   unsigned int flags;
 
-  if (decl && TREE_CODE (decl) == FUNCTION_DECL)
+  if (decl ? TREE_CODE (decl) == FUNCTION_DECL
+      : strncmp (name, ".text", 5) == 0)
     flags = SECTION_CODE;
   else if (decl)
     {

Re: [PATCH] PowerPC section type conflict

[Richard Henderson]

On 12/16/2011 03:16 AM, Chung-Lin Tang wrote:
> Hi, under powerpc targets, using -mrelocatable under some cases triggers
> an error during final assembly generation: rs6000_assemble_integer()
> calls varasm.c:unlikely_text_section_p(), and the call chain eventually
> gets to where the section htab is queried for ".text.unlikely", and
> fails because of mismatched section flags: "error: foo causes a section
> type conflict"
> 
> This seems to happen after rev.167085, 4.6 branch is also affected.
> 
> The flag mismatch seems quite straightforward: current_function_decl is
> passed in as the decl here, and as it's final assembly now, it is
> NULL_TREE. varasm.c:default_section_type_flags() adds SECTION_WRITE to
> its return value, and things get borked.

I don't understand what was put into .text.unlikely that was not a
function in the first place?  Did we try to put data there somewhere?


r~

Re: [PATCH] PowerPC section type conflict

[Chung-Lin Tang]

On 2011/12/17 06:21 AM, Richard Henderson wrote:
> On 12/16/2011 03:16 AM, Chung-Lin Tang wrote:
>> Hi, under powerpc targets, using -mrelocatable under some cases triggers
>> an error during final assembly generation: rs6000_assemble_integer()
>> calls varasm.c:unlikely_text_section_p(), and the call chain eventually
>> gets to where the section htab is queried for ".text.unlikely", and
>> fails because of mismatched section flags: "error: foo causes a section
>> type conflict"
>>
>> This seems to happen after rev.167085, 4.6 branch is also affected.
>>
>> The flag mismatch seems quite straightforward: current_function_decl is
>> passed in as the decl here, and as it's final assembly now, it is
>> NULL_TREE. varasm.c:default_section_type_flags() adds SECTION_WRITE to
>> its return value, and things get borked.
> 
> I don't understand what was put into .text.unlikely that was not a
> function in the first place?  Did we try to put data there somewhere?

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).

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).

Chung-Lin

Re: [PATCH] PowerPC section type conflict

[Richard Henderson]

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?

Re: [PATCH] PowerPC section type conflict (created PR 51623)

[Chung-Lin Tang]

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

Thanks,
Chung-Lin

Re: [PATCH] PowerPC section type conflict (created PR 51623)

[Michael Meissner]

[-- Attachment #1: Type: text/plain, Size: 3555 bytes --]

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.


-- 
Michael Meissner, IBM
5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA
meissner@linux.vnet.ibm.com	fax +1 (978) 399-6899

[-- Attachment #2: bug-51623.patch01b --]
[-- Type: text/plain, Size: 3539 bytes --]

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;

Re: [PATCH] PowerPC section type conflict (created PR 51623)

[Richard Henderson]

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~

Re: [PATCH] PowerPC section type conflict (created PR 51623)

[Michael Meissner]

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.

-- 
Michael Meissner, IBM
5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA
meissner@linux.vnet.ibm.com	fax +1 (978) 399-6899

Re: [PATCH] PowerPC section type conflict (created PR 51623)

[Michael Meissner]

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.

In talking to David, I convinced myself that initialized pointers should never
be in the text section (switch statements use relative pointers, not absolute
addresses currently), so I committed this patch on top of the previous one:

2011-12-29  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/51623
	* config/rs6000/rs6000.c (rs6000_assemble_integer): Delete check
	for an initialized pointer being in a text section with
	-mrelocatable, since it should never happen.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 182730)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -15456,12 +15456,13 @@ rs6000_assemble_integer (rtx x, unsigned
     {
       static int recurse = 0;
 
-      /* For -mrelocatable, we mark all addresses that need to be fixed up
-	 in the .fixup section.  */
+      /* For -mrelocatable, we mark all addresses that need to be fixed up in
+	 the .fixup section.  Since the TOC section is already relocated, we
+	 don't need to mark it here.  We used to skip the text section, but it
+	 should never be valid for relocated addresses to be placed in the text
+	 section.  */
       if (TARGET_RELOCATABLE
 	  && in_section != toc_section
-	  && in_section != text_section
-	  && (in_section && (in_section->common.flags & SECTION_CODE)) == 0
 	  && !recurse
 	  && GET_CODE (x) != CONST_INT
 	  && GET_CODE (x) != CONST_DOUBLE


-- 
Michael Meissner, IBM
5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA
meissner@linux.vnet.ibm.com	fax +1 (978) 399-6899