Fix PR 93242: patchable-function-entry broken on MIPS
diff mbox series

Message ID 1579308383-22325-1-git-send-email-apinski@marvell.com
State New
Headers show
Series
  • Fix PR 93242: patchable-function-entry broken on MIPS
Related show

Commit Message

Andrew Pinski Jan. 18, 2020, 12:46 a.m. UTC
From: Andrew Pinski <apinski@marvell.com>

On MIPS, .set noreorder/reorder needs to emitted around
the nop.  The template for the nop instruction uses %(/%) to
do that.  But default_print_patchable_function_entry uses
fprintf rather than output_asm_insn to output the instruction.

This fixes the problem by using output_asm_insn to emit the nop
instruction.

OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
mips toolchain also.

Thanks,
Andrew Pinski

ChangeLog:

* targhooks.c (default_print_patchable_function_entry): Use
output_asm_insn to emit the nop instruction.

Change-Id: I9d7cff2fc227a41461b9068e3af1fd3a5a9c059b
---
 gcc/targhooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Richard Biener Jan. 20, 2020, 8:42 a.m. UTC | #1
On Sat, Jan 18, 2020 at 1:47 AM <apinski@marvell.com> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> On MIPS, .set noreorder/reorder needs to emitted around
> the nop.  The template for the nop instruction uses %(/%) to
> do that.  But default_print_patchable_function_entry uses
> fprintf rather than output_asm_insn to output the instruction.
>
> This fixes the problem by using output_asm_insn to emit the nop
> instruction.
>
> OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
> mips toolchain also.

OK.

Richard.

> Thanks,
> Andrew Pinski
>
> ChangeLog:
>
> * targhooks.c (default_print_patchable_function_entry): Use
> output_asm_insn to emit the nop instruction.
>
> Change-Id: I9d7cff2fc227a41461b9068e3af1fd3a5a9c059b
> ---
>  gcc/targhooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 4819bb8..415c21b 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1822,7 +1822,7 @@ default_print_patchable_function_entry (FILE *file,
>
>    unsigned i;
>    for (i = 0; i < patch_area_size; ++i)
> -    fprintf (file, "\t%s\n", nop_templ);
> +    output_asm_insn (nop_templ, NULL);
>  }
>
>  bool
> --
> 1.8.3.1
>
Jeff Law Jan. 22, 2020, 8:48 p.m. UTC | #2
On Mon, 2020-01-20 at 09:42 +0100, Richard Biener wrote:
> On Sat, Jan 18, 2020 at 1:47 AM <apinski@marvell.com> wrote:
> > From: Andrew Pinski <apinski@marvell.com>
> > 
> > On MIPS, .set noreorder/reorder needs to emitted around
> > the nop.  The template for the nop instruction uses %(/%) to
> > do that.  But default_print_patchable_function_entry uses
> > fprintf rather than output_asm_insn to output the instruction.
> > 
> > This fixes the problem by using output_asm_insn to emit the nop
> > instruction.
> > 
> > OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
> > mips toolchain also.
> 
> OK.
FWIW, I think this may have broke the arc-elf port.  I'm getting
failures for the patchable function entry tests.  It looks like the
port wants to peek a the current_output_insn in its handling of an
output punctuation characters and current_output_insn is NULL.

jeff
>
Andrew Pinski Jan. 22, 2020, 9:16 p.m. UTC | #3
On Wed, Jan 22, 2020 at 12:48 PM Jeff Law <law@redhat.com> wrote:
>
> On Mon, 2020-01-20 at 09:42 +0100, Richard Biener wrote:
> > On Sat, Jan 18, 2020 at 1:47 AM <apinski@marvell.com> wrote:
> > > From: Andrew Pinski <apinski@marvell.com>
> > >
> > > On MIPS, .set noreorder/reorder needs to emitted around
> > > the nop.  The template for the nop instruction uses %(/%) to
> > > do that.  But default_print_patchable_function_entry uses
> > > fprintf rather than output_asm_insn to output the instruction.
> > >
> > > This fixes the problem by using output_asm_insn to emit the nop
> > > instruction.
> > >
> > > OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
> > > mips toolchain also.
> >
> > OK.
> FWIW, I think this may have broke the arc-elf port.  I'm getting
> failures for the patchable function entry tests.  It looks like the
> port wants to peek a the current_output_insn in its handling of an
> output punctuation characters and current_output_insn is NULL.

I suspect arc-elf was failing beforehand; just not crashing the compiler :).
Before this patch we would be printing out "nop%?" for arc-elf.  The
tests are "compile" so they would have "passed" but only because the
tests was not trying to assemble them.  If someone had tried to use
this option of arc-elf, they would have ran into a similar problem as
mips, printing out %? (in arc case).

Thanks,
Andrew Pinski


>
> jeff
> >
>
Andrew Pinski Jan. 22, 2020, 9:33 p.m. UTC | #4
On Wed, Jan 22, 2020 at 1:16 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Wed, Jan 22, 2020 at 12:48 PM Jeff Law <law@redhat.com> wrote:
> >
> > On Mon, 2020-01-20 at 09:42 +0100, Richard Biener wrote:
> > > On Sat, Jan 18, 2020 at 1:47 AM <apinski@marvell.com> wrote:
> > > > From: Andrew Pinski <apinski@marvell.com>
> > > >
> > > > On MIPS, .set noreorder/reorder needs to emitted around
> > > > the nop.  The template for the nop instruction uses %(/%) to
> > > > do that.  But default_print_patchable_function_entry uses
> > > > fprintf rather than output_asm_insn to output the instruction.
> > > >
> > > > This fixes the problem by using output_asm_insn to emit the nop
> > > > instruction.
> > > >
> > > > OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
> > > > mips toolchain also.
> > >
> > > OK.
> > FWIW, I think this may have broke the arc-elf port.  I'm getting
> > failures for the patchable function entry tests.  It looks like the
> > port wants to peek a the current_output_insn in its handling of an
> > output punctuation characters and current_output_insn is NULL.
>
> I suspect arc-elf was failing beforehand; just not crashing the compiler :).
> Before this patch we would be printing out "nop%?" for arc-elf.  The
> tests are "compile" so they would have "passed" but only because the
> tests was not trying to assemble them.  If someone had tried to use
> this option of arc-elf, they would have ran into a similar problem as
> mips, printing out %? (in arc case).

Just a quick survey of the targets which had an issue before this patch:
* arc - uses %?
* bpf - uses %%
* mips - uses %(/%) which emits .set noreorder/reorder
* nios2 - %.
* s390 - %%

nios2 - checks current_output_insn for null-ness when it is seeing if
it needs to print . or not.
arc - does not check current_output_insn for null-ness

Thanks,
Andrew Pinski


>
> Thanks,
> Andrew Pinski
>
>
> >
> > jeff
> > >
> >
Jeff Law Jan. 22, 2020, 10:46 p.m. UTC | #5
On Wed, 2020-01-22 at 13:16 -0800, Andrew Pinski wrote:
> On Wed, Jan 22, 2020 at 12:48 PM Jeff Law <law@redhat.com> wrote:
> > On Mon, 2020-01-20 at 09:42 +0100, Richard Biener wrote:
> > > On Sat, Jan 18, 2020 at 1:47 AM <apinski@marvell.com> wrote:
> > > > From: Andrew Pinski <apinski@marvell.com>
> > > > 
> > > > On MIPS, .set noreorder/reorder needs to emitted around
> > > > the nop.  The template for the nop instruction uses %(/%) to
> > > > do that.  But default_print_patchable_function_entry uses
> > > > fprintf rather than output_asm_insn to output the instruction.
> > > > 
> > > > This fixes the problem by using output_asm_insn to emit the nop
> > > > instruction.
> > > > 
> > > > OK?  Bootstrapped and tested on x86_64-linux-gnu and built a full
> > > > mips toolchain also.
> > > 
> > > OK.
> > FWIW, I think this may have broke the arc-elf port.  I'm getting
> > failures for the patchable function entry tests.  It looks like the
> > port wants to peek a the current_output_insn in its handling of an
> > output punctuation characters and current_output_insn is NULL.
> 
> I suspect arc-elf was failing beforehand; just not crashing the compiler :).
> Before this patch we would be printing out "nop%?" for arc-elf.  The
> tests are "compile" so they would have "passed" but only because the
> tests was not trying to assemble them.  If someone had tried to use
> this option of arc-elf, they would have ran into a similar problem as
> mips, printing out %? (in arc case).
The tester complains when something changes from pass to fail, it does
not complain for new tests that fail.

Jeff

Patch
diff mbox series

diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 4819bb8..415c21b 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1822,7 +1822,7 @@  default_print_patchable_function_entry (FILE *file,
 
   unsigned i;
   for (i = 0; i < patch_area_size; ++i)
-    fprintf (file, "\t%s\n", nop_templ);
+    output_asm_insn (nop_templ, NULL);
 }
 
 bool