[Patch, PR 60158] Generate .fixup sections for .data.rel.ro.local entries.

["rohitarulraj at freescale dot com" <rohitarulraj at freescale dot com>]

Hello All,

This is related to the following bug:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60158

Test case: Refer below
Target: e500v2
Command line options: -Os -fdata-sections -fpic -mrelocatable -mno-spe
Notes:
         GCC v4.7.3 puts the address of "abc" into the GOT directly
         GCC v4.8.2 is generating a pointer to the string and putting the address of that in the GOT.  But it doesn't generate the required ".fixup" table entries.


asm code generated:

...
        .section        .data.rel.ro.local,"aw",@progbits
        .align 2
.LC5:
        .4byte  .LC0
....
...
        .section        .rodata.str1.4,"aMS",@progbits,1
        .align 2
.LC0:
        .string "abc"
.LC1:
        .string "def"

..
1) Compared to GCC v4.7.3, with GCC v4.8 series, there is a new flag '-fira-hoist-pressure' which is enabled by default at -Os.
    Disabling this optimization '-fno-ira-hoist-pressure' generates similar code as in GCC v4.7.3

2) The actual issue is that with GCC v4.8.2, the move instruction of that string constant ".LC0" is getting spilled. The reload pass, for any constants that aren't allowed and can't be reloaded in to registers tries to change them into memory references. Then while emitting that string constant to asm code (A:varasm.c: output_constant_pool_1), it explicitly passes the alignment as 1 which prevents the generation of fix-up table entries in  'B: rs6000.c:rs6000_assemble_integer' because the data is considered unaligned now.

Source file: gcc-4.8.2/gcc/varasm.c
@@ -7120,7 +7120,7 @@
       if (CONSTANT_POOL_ADDRESS_P (symbol))
        {
          desc = SYMBOL_REF_CONSTANT (symbol);
          output_constant_pool_1 (desc, 1);                             ------------- (A)
          offset += GET_MODE_SIZE (desc->mode);
        }
       else if (TREE_CONSTANT_POOL_ADDRESS_P (symbol))

Source file: gcc-4.8.2/gcc/config/rs6000.c
static bool
rs6000_assemble_integer (rtx x, unsigned int size, int aligned_p)
{
#ifdef RELOCATABLE_NEEDS_FIXUP
  /* Special handling for SI values.  */
  if (RELOCATABLE_NEEDS_FIXUP && size == 4 && aligned_p)    ----------------------- (B)
    {


Ideally, passing proper alignment to (A) should have worked. But if we pass the proper alignment to (A) output_constant_pool_1, .align directive gets emitted twice. Is that the actual purpose of explicitly passing '1' as alignment to output_constant_pool_1?
-         output_constant_pool_1 (desc, 1);
+        output_constant_pool_1 (desc, desc->align);


Generated asm with this change:
        .section        .data.rel.ro.local,"aw",@progbits  
        .align 2                                          
        .align 2                                          
.LC5:                                                      
.LCP0:                                                    
        .long   (.LC0)@fixup                              

Adding a similar change to its helper function "output_constant_pool_2" does generate the expected code.

[gcc]
2014-04-22  Rohit  <rohitarulraj@freescale.com>

    PR target/60158
    * varasm.c (output_constant_pool_1): Pass actual alignment value to output_constant_pool_2
      to emit ".fixup" section.

[gcc/testsuite]
2014-04-22  Rohit  <rohitarulraj@freescale.com>

    PR target/60158
    * gcc.target/powerpc/pr60158.c: New test.  Check if ".fixup" section gets emitted for
     ".data.rel.ro.local" section.


Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c        (revision 209534)
+++ gcc/varasm.c        (working copy)
@@ -3771,7 +3771,7 @@ output_constant_pool_1 (struct constant_
   targetm.asm_out.internal_label (asm_out_file, "LC", desc->labelno);

   /* Output the data.  */
-  output_constant_pool_2 (desc->mode, x, align);
+  output_constant_pool_2 (desc->mode, x, desc->align);

   /* Make sure all constants in SECTION_MERGE and not SECTION_STRINGS
      sections have proper size.  */
Index: gcc/testsuite/gcc.target/powerpc/pr60158.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr60158.c  (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr60158.c  (revision 0)
@@ -0,0 +1,91 @@
+/* { dg-do compile } */
+/* { dg-skip-if "not an SPE target" { ! powerpc_spe_nocache } { "*" } { "" } } */
+/* { dg-options "-mcpu=8548 -mno-spe -mfloat-gprs=double -Os -fdata-sections -fpic -mrelocatable" } */
+
+#define NULL 0
+int func (int val);
+void *func2 (void *ptr);
+
+static const char *ifs;
+static char map[256];
+
+typedef struct {
+/*
+ * None of these fields are used, but removing any
+ * of them makes the problem go away.
+ */
+  char *data;
+  int length;
+  int maxlen;
+  int quote;
+} o_string;
+
+#define NULL_O_STRING {NULL,0,0,0}
+
+static int parse_stream (void *dest, void *ctx)
+{
+  int ch = func (0), m;
+
+  while (ch != -1) {
+    m = map[ch];
+    if (ch != '\n')
+    func2(dest);
+
+    ctx = func2 (ctx);
+    if (!func (0))
+      return 0;
+    if (m != ch) {
+      func2 ("htns");
+      break;
+    }
+  }
+  return -1;
+}
+
+static void mapset (const char *set, int code)
+{
+  const char *s;
+  for (s=set; *s; s++)  map[(int)*s] = code;
+}
+
+static void update_ifs_map(void)
+{
+  /* char *ifs and char map[256] are both globals. */
+  ifs = func2 ("abc");
+  if (ifs == NULL) ifs="def";
+
+  func2 (map);
+  {
+    char subst[2] = {4, 0};
+    mapset (subst, 3);
+  }
+  mapset (";&|#", 1);
+}
+
+int parse_stream_outer (int flag)
+{
+  int blah;
+  o_string temp=NULL_O_STRING;
+  int rcode;
+
+  do {
+    update_ifs_map ();
+    func2 (&blah); /* a memory clobber works as well */
+    rcode = parse_stream (&temp, NULL);
+    func2 ("aoeu");
+    if (func (0) != 0) {
+      func2 (NULL);
+    }
+  } while (rcode != -1);
+  return 0;
+}
+
+/* { dg-final { if ![file exists pr60158.s] { fail "pr60158.c (compile)"; return; } } } */
+
+/* { dg-final { set c_rel [llength [grep pr60158.s \\.data\\.rel\\.ro\\.local]] } } */
+/* { dg-final { set c_fix [llength [grep pr60158.s \\.fixup]] } } */
+/* { dg-final { if [string match $c_rel $c_fix] \{                     } } */
+/* { dg-final {     pass "pr60158.c (passed)"  } } */
+/* { dg-final { \} else \{                                     } } */
+/* { dg-final {     fail "pr60158.c (.fixup table entries not generated for .data.rel.ro.local section)"       } } */
+/* { dg-final { \}

Tested on e500v2, e500mc, e5500 linux tool chains with no new regressions.

Any comments?

Regards,
Rohit