diff mbox

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

Message ID d0a7b88818ff4272a92e2cd6b49f12f5@DM2PR03MB352.namprd03.prod.outlook.com
State New
Headers show

Commit Message

rohitarulraj@freescale.com April 25, 2014, 2:57 p.m. UTC 
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.



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

Any comments?

Regards,
Rohit

Comments

Alan Modra April 26, 2014, 6:22 a.m. UTC | #1 
On Fri, Apr 25, 2014 at 02:57:38PM +0000, rohitarulraj@freescale.com wrote:
> 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);

I think the reason 1 is passed here for align is that with
-fsection-anchors, in output_object_block we've already laid out
everything in the block, assigning offsets from the start of the
block.  Aligning shouldn't be necessary, because we've already done
that..  OTOH, it shouldn't hurt to align again.
rohitarulraj@freescale.com May 8, 2014, 1:54 p.m. UTC | #2 
> -----Original Message-----
> From: Alan Modra [mailto:amodra@gmail.com]
> Sent: Saturday, April 26, 2014 11:52 AM
> To: Dharmakan Rohit-B30502
> Cc: gcc-patches@gcc.gnu.org; dje.gcc@gmail.com; Wienskoski Edmar-RA8797
> Subject: Re: [Patch, PR 60158] Generate .fixup sections for
> .data.rel.ro.local entries.
> 
> On Fri, Apr 25, 2014 at 02:57:38PM +0000, rohitarulraj@freescale.com
> wrote:
> > 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);
> 
> I think the reason 1 is passed here for align is that with -fsection-
> anchors, in output_object_block we've already laid out everything in the
> block, assigning offsets from the start of the block.  Aligning shouldn't
> be necessary, because we've already done that..  OTOH, it shouldn't hurt
> to align again.
> 
Thanks. I have tested for both the cases on e500v2, e500mc, e5500, ppc64 (GCC v4.8.2 branch) with no regressions.

Patch1 [gcc.fix_pr60158_fixup_table-fsf]: Pass actual alignment value to output_constant_pool_2.
Patch2 [gcc.fix_pr60158_fixup_table-fsf-2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
        (Note: this generates ".align" directive twice).

Is it ok to commit? Any comments?

Regards,
Rohit
David Edelsohn May 8, 2014, 3:57 p.m. UTC | #3 
Rohit,

The subject line and thread may confuse people that this is a
PowerPC-specific issue. You need approval from a reviewer with
authority over varasm.c.

Thanks, David

On Thu, May 8, 2014 at 9:54 AM, rohitarulraj@freescale.com
<rohitarulraj@freescale.com> wrote:
>> -----Original Message-----
>> From: Alan Modra [mailto:amodra@gmail.com]
>> Sent: Saturday, April 26, 2014 11:52 AM
>> To: Dharmakan Rohit-B30502
>> Cc: gcc-patches@gcc.gnu.org; dje.gcc@gmail.com; Wienskoski Edmar-RA8797
>> Subject: Re: [Patch, PR 60158] Generate .fixup sections for
>> .data.rel.ro.local entries.
>>
>> On Fri, Apr 25, 2014 at 02:57:38PM +0000, rohitarulraj@freescale.com
>> wrote:
>> > 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);
>>
>> I think the reason 1 is passed here for align is that with -fsection-
>> anchors, in output_object_block we've already laid out everything in the
>> block, assigning offsets from the start of the block.  Aligning shouldn't
>> be necessary, because we've already done that..  OTOH, it shouldn't hurt
>> to align again.
>>
> Thanks. I have tested for both the cases on e500v2, e500mc, e5500, ppc64 (GCC v4.8.2 branch) with no regressions.
>
> Patch1 [gcc.fix_pr60158_fixup_table-fsf]: Pass actual alignment value to output_constant_pool_2.
> Patch2 [gcc.fix_pr60158_fixup_table-fsf-2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
>         (Note: this generates ".align" directive twice).
>
> Is it ok to commit? Any comments?
>
> Regards,
> Rohit
>
rohitarulraj@freescale.com June 4, 2014, 11:16 a.m. UTC | #4 
Ping.
I have changed the subject line accordingly.

Regards,
Rohit

> -----Original Message-----

> From: David Edelsohn [mailto:dje.gcc@gmail.com]

> Sent: Thursday, May 08, 2014 9:28 PM

> To: Dharmakan Rohit-B30502; Jakub Jelinek; Richard Biener

> Cc: Alan Modra; gcc-patches@gcc.gnu.org; Wienskoski Edmar-RA8797

> Subject: Re: [Patch, PR 60158] Generate .fixup sections for

> .data.rel.ro.local entries.

> 

> Rohit,

> 

> The subject line and thread may confuse people that this is a PowerPC-

> specific issue. You need approval from a reviewer with authority over

> varasm.c.

> 

> Thanks, David

> 

> On Thu, May 8, 2014 at 9:54 AM, rohitarulraj@freescale.com

> <rohitarulraj@freescale.com> wrote:

> >> -----Original Message-----

> >> From: Alan Modra [mailto:amodra@gmail.com]

> >> Sent: Saturday, April 26, 2014 11:52 AM

> >> To: Dharmakan Rohit-B30502

> >> Cc: gcc-patches@gcc.gnu.org; dje.gcc@gmail.com; Wienskoski

> >> Edmar-RA8797

> >> Subject: Re: [Patch, PR 60158] Generate .fixup sections for

> >> .data.rel.ro.local entries.

> >>

> >> On Fri, Apr 25, 2014 at 02:57:38PM +0000, rohitarulraj@freescale.com

> >> wrote:

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

> >>

> >> I think the reason 1 is passed here for align is that with -fsection-

> >> anchors, in output_object_block we've already laid out everything in

> >> the block, assigning offsets from the start of the block.  Aligning

> >> shouldn't be necessary, because we've already done that..  OTOH, it

> >> shouldn't hurt to align again.

> >>

> > Thanks. I have tested for both the cases on e500v2, e500mc, e5500,

> ppc64 (GCC v4.8.2 branch) with no regressions.

> >

> > Patch1 [gcc.fix_pr60158_fixup_table-fsf]: Pass actual alignment value

> to output_constant_pool_2.

> > Patch2 [gcc.fix_pr60158_fixup_table-fsf-2]: Use the alignment data

> available in the first argument (constant_descriptor_rtx) of

> output_constant_pool_1.

> >         (Note: this generates ".align" directive twice).

> >

> > Is it ok to commit? Any comments?

> >

> > Regards,

> > Rohit

> >
diff mbox

Patch

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 { \}