Issue 6572065: [google] Emit relative addresses to function patch sections instead of absolute addresses.

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 7 months ago by harshit

Modified:
11 years, 6 months ago

Reviewers:
Diego Novillo, davidxl, ppluzhnikov

CC:
gcc-patches_gcc.gnu.org

Visibility:
Public.

Description

commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6 Author: Harshit Chopra <harshit@google.com> Date: Thu Sep 20 17:49:59 2012 -0700 Instead of emitting absolute addresses to the function patch sections, emit relative addresses. Absolute addresses might require relocation, which is time consuming and fraught with other issues. M gcc/config/i386/i386.c Tested: Ran make check-gcc and manually confirmed that the affected tests pass. ChangeLog: 2012-09-28 Harshit Chopra <harshit@google.com> * gcc/config/i386/i386.c (ix86_output_function_nops_prologue_epilogue): Emit relative address to function patch sections.

Patch Set 1 #

Created: 11 years, 7 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+5 lines, -2 lines)			Patch
	M	gcc/config/i386/i386.c	View		3 chunks	+5 lines, -2 lines	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

harshit

commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6 Author: Harshit Chopra <harshit@google.com> Date: Thu Sep 20 17:49:59 2012 -0700 Instead of ...

11 years, 7 months ago (2012-09-28 09:24:39 UTC) #1

commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
Author: Harshit Chopra <harshit@google.com>
Date:   Thu Sep 20 17:49:59 2012 -0700

    Instead of emitting absolute addresses to the function patch sections, emit
relative addresses. Absolute addresses might require relocation, which is time
consuming and fraught with other issues.

M	gcc/config/i386/i386.c

Tested:
  Ran make check-gcc and manually confirmed that the affected tests pass.

ChangeLog:

2012-09-28  Harshit Chopra  <harshit@google.com>

	* gcc/config/i386/i386.c (ix86_output_function_nops_prologue_epilogue): Emit
relative address to function patch sections.

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index f72b0b5..8c9334f 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
        $LFPEL0:
          <pre_instruction>
          0x90 (repeated num_actual_nops times)
-         .quad $LFPESL0
+         .quad $LFPESL0 - .
      followed by section 'section_name' which contains the address
      of instruction at 'label'.
    */
@@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
     asm_fprintf (file, ASM_BYTE"0x90\n");
 
   fprintf (file, ASM_QUAD);
+  /* Output "section_label - ." for the relative address of the entry in
+     the section 'section_name'.  */
   assemble_name_raw (file, section_label);
+  fprintf (file, " - .");
   fprintf (file, "\n");
 
   /* Emit the backpointer section. For functions belonging to comdat group,
@@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
          .quad $LFPEL0
    */
   ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
-  fprintf(file, ASM_QUAD"\t");
+  fprintf(file, ASM_QUAD);
   assemble_name_raw (file, label);
   fprintf (file, "\n");
 

--
This patch is available for review at http://codereview.appspot.com/6572065

harshit

Ping! -- Harshit On Fri, Sep 28, 2012 at 2:24 AM, Harshit Chopra <harshit@google.com> wrote: ...

11 years, 6 months ago (2012-10-05 22:10:19 UTC) #2

Ping!


--
Harshit


On Fri, Sep 28, 2012 at 2:24 AM, Harshit Chopra <harshit@google.com> wrote:

> commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
> Author: Harshit Chopra <harshit@google.com>
> Date:   Thu Sep 20 17:49:59 2012 -0700
>
>     Instead of emitting absolute addresses to the function patch sections,
> emit relative addresses. Absolute addresses might require relocation, which
> is time consuming and fraught with other issues.
>
> M       gcc/config/i386/i386.c
>
> Tested:
>   Ran make check-gcc and manually confirmed that the affected tests pass.
>
> ChangeLog:
>
> 2012-09-28  Harshit Chopra  <harshit@google.com>
>
>         * gcc/config/i386/i386.c
> (ix86_output_function_nops_prologue_epilogue): Emit relative address to
> function patch sections.
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f72b0b5..8c9334f 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
> *file,
>         $LFPEL0:
>           <pre_instruction>
>           0x90 (repeated num_actual_nops times)
> -         .quad $LFPESL0
> +         .quad $LFPESL0 - .
>       followed by section 'section_name' which contains the address
>       of instruction at 'label'.
>     */
> @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
> *file,
>      asm_fprintf (file, ASM_BYTE"0x90\n");
>
>    fprintf (file, ASM_QUAD);
> +  /* Output "section_label - ." for the relative address of the entry in
> +     the section 'section_name'.  */
>    assemble_name_raw (file, section_label);
> +  fprintf (file, " - .");
>    fprintf (file, "\n");
>
>    /* Emit the backpointer section. For functions belonging to comdat
> group,
> @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
> *file,
>           .quad $LFPEL0
>     */
>    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
> -  fprintf(file, ASM_QUAD"\t");
> +  fprintf(file, ASM_QUAD);
>    assemble_name_raw (file, label);
>    fprintf (file, "\n");
>
>
> --
> This patch is available for review at
> http://codereview.appspot.com/6572065
>

Diego Novillo

Harshit, why didn't you propose this patch for trunk? Why should we make it a ...

11 years, 6 months ago (2012-10-05 22:53:59 UTC) #3

Harshit, why didn't you propose this patch for trunk?  Why should we
make it a google-local patch?


Diego.

On Fri, Sep 28, 2012 at 5:24 AM, Harshit Chopra <harshit@google.com> wrote:
> commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
> Author: Harshit Chopra <harshit@google.com>
> Date:   Thu Sep 20 17:49:59 2012 -0700
>
>     Instead of emitting absolute addresses to the function patch sections,
emit relative addresses. Absolute addresses might require relocation, which is
time consuming and fraught with other issues.
>
> M       gcc/config/i386/i386.c
>
> Tested:
>   Ran make check-gcc and manually confirmed that the affected tests pass.
>
> ChangeLog:
>
> 2012-09-28  Harshit Chopra  <harshit@google.com>
>
>         * gcc/config/i386/i386.c
(ix86_output_function_nops_prologue_epilogue): Emit relative address to function
patch sections.
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f72b0b5..8c9334f 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>         $LFPEL0:
>           <pre_instruction>
>           0x90 (repeated num_actual_nops times)
> -         .quad $LFPESL0
> +         .quad $LFPESL0 - .
>       followed by section 'section_name' which contains the address
>       of instruction at 'label'.
>     */
> @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>      asm_fprintf (file, ASM_BYTE"0x90\n");
>
>    fprintf (file, ASM_QUAD);
> +  /* Output "section_label - ." for the relative address of the entry in
> +     the section 'section_name'.  */
>    assemble_name_raw (file, section_label);
> +  fprintf (file, " - .");
>    fprintf (file, "\n");
>
>    /* Emit the backpointer section. For functions belonging to comdat group,
> @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>           .quad $LFPEL0
>     */
>    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
> -  fprintf(file, ASM_QUAD"\t");
> +  fprintf(file, ASM_QUAD);
>    assemble_name_raw (file, label);
>    fprintf (file, "\n");
>
>
> --
> This patch is available for review at http://codereview.appspot.com/6572065

Diego Novillo

Harshit, why didn't you propose this patch for trunk? Why should we make it a ...

11 years, 6 months ago (2012-10-05 22:54:00 UTC) #4

Harshit, why didn't you propose this patch for trunk?  Why should we
make it a google-local patch?


Diego.

On Fri, Sep 28, 2012 at 5:24 AM, Harshit Chopra <harshit@google.com> wrote:
> commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
> Author: Harshit Chopra <harshit@google.com>
> Date:   Thu Sep 20 17:49:59 2012 -0700
>
>     Instead of emitting absolute addresses to the function patch sections,
emit relative addresses. Absolute addresses might require relocation, which is
time consuming and fraught with other issues.
>
> M       gcc/config/i386/i386.c
>
> Tested:
>   Ran make check-gcc and manually confirmed that the affected tests pass.
>
> ChangeLog:
>
> 2012-09-28  Harshit Chopra  <harshit@google.com>
>
>         * gcc/config/i386/i386.c
(ix86_output_function_nops_prologue_epilogue): Emit relative address to function
patch sections.
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f72b0b5..8c9334f 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>         $LFPEL0:
>           <pre_instruction>
>           0x90 (repeated num_actual_nops times)
> -         .quad $LFPESL0
> +         .quad $LFPESL0 - .
>       followed by section 'section_name' which contains the address
>       of instruction at 'label'.
>     */
> @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>      asm_fprintf (file, ASM_BYTE"0x90\n");
>
>    fprintf (file, ASM_QUAD);
> +  /* Output "section_label - ." for the relative address of the entry in
> +     the section 'section_name'.  */
>    assemble_name_raw (file, section_label);
> +  fprintf (file, " - .");
>    fprintf (file, "\n");
>
>    /* Emit the backpointer section. For functions belonging to comdat group,
> @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>           .quad $LFPEL0
>     */
>    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
> -  fprintf(file, ASM_QUAD"\t");
> +  fprintf(file, ASM_QUAD);
>    assemble_name_raw (file, label);
>    fprintf (file, "\n");
>
>
> --
> This patch is available for review at http://codereview.appspot.com/6572065

Diego Novillo

On Fri, Oct 5, 2012 at 6:53 PM, Diego Novillo <dnovillo@google.com> wrote: > Harshit, why ...

11 years, 6 months ago (2012-10-05 23:13:48 UTC) #5

harshit

Okay. I will ping the upstream patch along with the modifications that I have done ...

11 years, 6 months ago (2012-10-05 23:39:12 UTC) #6

davidxl

xray feature is not in trunk yet. David On Fri, Oct 5, 2012 at 3:53 ...

11 years, 6 months ago (2012-10-06 17:13:39 UTC) #7

xray feature is not in trunk yet.

David

On Fri, Oct 5, 2012 at 3:53 PM, Diego Novillo <dnovillo@google.com> wrote:
> Harshit, why didn't you propose this patch for trunk?  Why should we
> make it a google-local patch?
>
>
> Diego.
>
> On Fri, Sep 28, 2012 at 5:24 AM, Harshit Chopra <harshit@google.com> wrote:
>> commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
>> Author: Harshit Chopra <harshit@google.com>
>> Date:   Thu Sep 20 17:49:59 2012 -0700
>>
>>     Instead of emitting absolute addresses to the function patch sections,
emit relative addresses. Absolute addresses might require relocation, which is
time consuming and fraught with other issues.
>>
>> M       gcc/config/i386/i386.c
>>
>> Tested:
>>   Ran make check-gcc and manually confirmed that the affected tests pass.
>>
>> ChangeLog:
>>
>> 2012-09-28  Harshit Chopra  <harshit@google.com>
>>
>>         * gcc/config/i386/i386.c
(ix86_output_function_nops_prologue_epilogue): Emit relative address to function
patch sections.
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index f72b0b5..8c9334f 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>>         $LFPEL0:
>>           <pre_instruction>
>>           0x90 (repeated num_actual_nops times)
>> -         .quad $LFPESL0
>> +         .quad $LFPESL0 - .
>>       followed by section 'section_name' which contains the address
>>       of instruction at 'label'.
>>     */
>> @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>>      asm_fprintf (file, ASM_BYTE"0x90\n");
>>
>>    fprintf (file, ASM_QUAD);
>> +  /* Output "section_label - ." for the relative address of the entry in
>> +     the section 'section_name'.  */
>>    assemble_name_raw (file, section_label);
>> +  fprintf (file, " - .");
>>    fprintf (file, "\n");
>>
>>    /* Emit the backpointer section. For functions belonging to comdat group,
>> @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>>           .quad $LFPEL0
>>     */
>>    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
>> -  fprintf(file, ASM_QUAD"\t");
>> +  fprintf(file, ASM_QUAD);
>>    assemble_name_raw (file, label);
>>    fprintf (file, "\n");
>>
>>
>> --
>> This patch is available for review at http://codereview.appspot.com/6572065

davidxl

Ok for google branches. Please consider resend the original xray patch to trunk (gcc-4_8) You ...

11 years, 6 months ago (2012-10-06 17:16:51 UTC) #8

Ok for google branches.

Please consider resend the original xray patch to trunk (gcc-4_8) You
need to make the runtime bits available publicly though.

thanks,

David

On Fri, Sep 28, 2012 at 2:24 AM, Harshit Chopra <harshit@google.com> wrote:
> commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
> Author: Harshit Chopra <harshit@google.com>
> Date:   Thu Sep 20 17:49:59 2012 -0700
>
>     Instead of emitting absolute addresses to the function patch sections,
emit relative addresses. Absolute addresses might require relocation, which is
time consuming and fraught with other issues.
>
> M       gcc/config/i386/i386.c
>
> Tested:
>   Ran make check-gcc and manually confirmed that the affected tests pass.
>
> ChangeLog:
>
> 2012-09-28  Harshit Chopra  <harshit@google.com>
>
>         * gcc/config/i386/i386.c
(ix86_output_function_nops_prologue_epilogue): Emit relative address to function
patch sections.
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f72b0b5..8c9334f 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>         $LFPEL0:
>           <pre_instruction>
>           0x90 (repeated num_actual_nops times)
> -         .quad $LFPESL0
> +         .quad $LFPESL0 - .
>       followed by section 'section_name' which contains the address
>       of instruction at 'label'.
>     */
> @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>      asm_fprintf (file, ASM_BYTE"0x90\n");
>
>    fprintf (file, ASM_QUAD);
> +  /* Output "section_label - ." for the relative address of the entry in
> +     the section 'section_name'.  */
>    assemble_name_raw (file, section_label);
> +  fprintf (file, " - .");
>    fprintf (file, "\n");
>
>    /* Emit the backpointer section. For functions belonging to comdat group,
> @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE
*file,
>           .quad $LFPEL0
>     */
>    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
> -  fprintf(file, ASM_QUAD"\t");
> +  fprintf(file, ASM_QUAD);
>    assemble_name_raw (file, label);
>    fprintf (file, "\n");
>
>
> --
> This patch is available for review at http://codereview.appspot.com/6572065

harshit

11 years, 6 months ago (2012-10-08 23:56:57 UTC) #9

Thanks for the review. Submitted to google/main branch. Do I also need to
explicitly submit this patch to google/gcc-4_7 branch?


--
Harshit


On Sat, Oct 6, 2012 at 10:16 AM, Xinliang David Li <davidxl@google.com>wrote:

> Ok for google branches.
>
> Please consider resend the original xray patch to trunk (gcc-4_8) You
> need to make the runtime bits available publicly though.
>

Will send an updated patch to trunk (with all the changes I have made since
I sent the original patch).


>
> thanks,
>
> David
>
> On Fri, Sep 28, 2012 at 2:24 AM, Harshit Chopra <harshit@google.com>
> wrote:
> > commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6
> > Author: Harshit Chopra <harshit@google.com>
> > Date:   Thu Sep 20 17:49:59 2012 -0700
> >
> >     Instead of emitting absolute addresses to the function patch
> sections, emit relative addresses. Absolute addresses might require
> relocation, which is time consuming and fraught with other issues.
> >
> > M       gcc/config/i386/i386.c
> >
> > Tested:
> >   Ran make check-gcc and manually confirmed that the affected tests pass.
> >
> > ChangeLog:
> >
> > 2012-09-28  Harshit Chopra  <harshit@google.com>
> >
> >         * gcc/config/i386/i386.c
> (ix86_output_function_nops_prologue_epilogue): Emit relative address to
> function patch sections.
> >
> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > index f72b0b5..8c9334f 100644
> > --- a/gcc/config/i386/i386.c
> > +++ b/gcc/config/i386/i386.c
> > @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue
> (FILE *file,
> >         $LFPEL0:
> >           <pre_instruction>
> >           0x90 (repeated num_actual_nops times)
> > -         .quad $LFPESL0
> > +         .quad $LFPESL0 - .
> >       followed by section 'section_name' which contains the address
> >       of instruction at 'label'.
> >     */
> > @@ -11110,7 +11110,10 @@ ix86_output_function_nops_prologue_epilogue
> (FILE *file,
> >      asm_fprintf (file, ASM_BYTE"0x90\n");
> >
> >    fprintf (file, ASM_QUAD);
> > +  /* Output "section_label - ." for the relative address of the entry in
> > +     the section 'section_name'.  */
> >    assemble_name_raw (file, section_label);
> > +  fprintf (file, " - .");
> >    fprintf (file, "\n");
> >
> >    /* Emit the backpointer section. For functions belonging to comdat
> group,
> > @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue
> (FILE *file,
> >           .quad $LFPEL0
> >     */
> >    ASM_OUTPUT_INTERNAL_LABEL (file, section_label);
> > -  fprintf(file, ASM_QUAD"\t");
> > +  fprintf(file, ASM_QUAD);
> >    assemble_name_raw (file, label);
> >    fprintf (file, "\n");
> >
> >
> > --
> > This patch is available for review at
> http://codereview.appspot.com/6572065
>

Expand All Messages | Collapse All Messages