diff mbox series

[powerpc] Fix unrecognized instruction errors with recent binutils

Message ID 20210925202746.819385-1-pc@us.ibm.com
State Committed
Commit ee874f44fd55988808a4a162ef21bfa2cc8dc6f7
Delegated to: Tulio Magno Quites Machado Filho
Headers show
Series [powerpc] Fix unrecognized instruction errors with recent binutils | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Paul A. Clarke Sept. 25, 2021, 8:27 p.m. UTC
Recent versions of binutils (with commit
b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a) stopped preserving "sticky"
options across a base `.machine` directive, nullifying the use of
passing "-many" through GCC to the assembler.  As a result, some
instructions which were recognized even under older, more stringent
`.machine` directives become unrecognized instructions in that
context.

In `sysdeps/powerpc/tst-set_ppr.c`, the use of the `mfppr32` extended
mnemonic became unrecognized, as the default compilation with GCC for
32bit powerpc adds a `.machine ppc` in the resulting assembly, so the
command line option `-Wa,-many` is essentially ignored, and the ISA 2.06
instructions and mnemonics, like `mfppr32`, are unrecognized.

The compilation of `sysdeps/powerpc/tst-set_ppr.c` fails with:
Error: unrecognized opcode: `mfppr32'

Add appropriate `.machine` directives in the assembly to bracket the
`mfppr32` instruction.

Part of a 2019 fix (commit 9250e6610fdb0f3a6f238d2813e319a41fb7a810) to
the above test's Makefile to add `-many` to the compilation when GCC
itself stopped passing `-many` to the assember no longer has any effect,
so remove that.

Reported-by: Joseph Myers <joseph@codesourcery.com>
---
 sysdeps/powerpc/Makefile      | 5 -----
 sysdeps/powerpc/tst-set_ppr.c | 3 ++-
 2 files changed, 2 insertions(+), 6 deletions(-)

Comments

Paul E Murphy Sept. 27, 2021, 5 p.m. UTC | #1
On 9/25/21 3:27 PM, Paul A. Clarke via Libc-alpha wrote:
> Recent versions of binutils (with commit
> b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a) stopped preserving "sticky"
> options across a base `.machine` directive, nullifying the use of
> passing "-many" through GCC to the assembler.  As a result, some
> instructions which were recognized even under older, more stringent
> `.machine` directives become unrecognized instructions in that
> context.
> 
> In `sysdeps/powerpc/tst-set_ppr.c`, the use of the `mfppr32` extended
> mnemonic became unrecognized, as the default compilation with GCC for
> 32bit powerpc adds a `.machine ppc` in the resulting assembly, so the
> command line option `-Wa,-many` is essentially ignored, and the ISA 2.06
> instructions and mnemonics, like `mfppr32`, are unrecognized.
> 
> The compilation of `sysdeps/powerpc/tst-set_ppr.c` fails with:
> Error: unrecognized opcode: `mfppr32'
> 
> Add appropriate `.machine` directives in the assembly to bracket the
> `mfppr32` instruction.
> 
> Part of a 2019 fix (commit 9250e6610fdb0f3a6f238d2813e319a41fb7a810) to
> the above test's Makefile to add `-many` to the compilation when GCC
> itself stopped passing `-many` to the assember no longer has any effect,
> so remove that.
> 
> Reported-by: Joseph Myers <joseph@codesourcery.com>
> ---
>   sysdeps/powerpc/Makefile      | 5 -----
>   sysdeps/powerpc/tst-set_ppr.c | 3 ++-
>   2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
> index 09860ffc0155..5e6cb07ce66d 100644
> --- a/sysdeps/powerpc/Makefile
> +++ b/sysdeps/powerpc/Makefile
> @@ -61,11 +61,6 @@ ifeq ($(subdir),misc)
>   sysdep_headers += sys/platform/ppc.h
>   tests += test-gettimebase
>   tests += tst-set_ppr
> -
> -# This test is expected to run and exit with EXIT_UNSUPPORTED on
> -# processors that do not implement the Power ISA 2.06 or greater.
> -# But the test makes use of instructions from Power ISA 2.06 and 2.07.
> -CFLAGS-tst-set_ppr.c += -Wa,-many
>   endif
> 
>   ifeq ($(subdir),wcsmbs)

OK.

> diff --git a/sysdeps/powerpc/tst-set_ppr.c b/sysdeps/powerpc/tst-set_ppr.c
> index 7684f5d6ea90..e80da1532063 100644
> --- a/sysdeps/powerpc/tst-set_ppr.c
> +++ b/sysdeps/powerpc/tst-set_ppr.c
> @@ -44,7 +44,8 @@ get_thread_priority (void)
>   {
>     /* Read the PPR.  */
>     ppr_t ppr;
> -  asm volatile (MFPPR" %0" : "=r"(ppr));
> +  asm volatile (".machine push; .machine power7; "MFPPR" %0; .machine pop"

Does it matter if power7 is quoted? LGTM to me otherwise.
Paul A. Clarke Sept. 27, 2021, 6:54 p.m. UTC | #2
On Mon, Sep 27, 2021 at 12:00:45PM -0500, Paul E Murphy via Libc-alpha wrote:
> On 9/25/21 3:27 PM, Paul A. Clarke via Libc-alpha wrote:
> > Recent versions of binutils (with commit
> > b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a) stopped preserving "sticky"
> > options across a base `.machine` directive, nullifying the use of
> > passing "-many" through GCC to the assembler.  As a result, some
> > instructions which were recognized even under older, more stringent
> > `.machine` directives become unrecognized instructions in that
> > context.
> > 
> > In `sysdeps/powerpc/tst-set_ppr.c`, the use of the `mfppr32` extended
> > mnemonic became unrecognized, as the default compilation with GCC for
> > 32bit powerpc adds a `.machine ppc` in the resulting assembly, so the
> > command line option `-Wa,-many` is essentially ignored, and the ISA 2.06
> > instructions and mnemonics, like `mfppr32`, are unrecognized.
> > 
> > The compilation of `sysdeps/powerpc/tst-set_ppr.c` fails with:
> > Error: unrecognized opcode: `mfppr32'
> > 
> > Add appropriate `.machine` directives in the assembly to bracket the
> > `mfppr32` instruction.
> > 
> > Part of a 2019 fix (commit 9250e6610fdb0f3a6f238d2813e319a41fb7a810) to
> > the above test's Makefile to add `-many` to the compilation when GCC
> > itself stopped passing `-many` to the assember no longer has any effect,
> > so remove that.
> > 
> > Reported-by: Joseph Myers <joseph@codesourcery.com>
> > ---
> >   sysdeps/powerpc/Makefile      | 5 -----
> >   sysdeps/powerpc/tst-set_ppr.c | 3 ++-
> >   2 files changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
> > index 09860ffc0155..5e6cb07ce66d 100644
> > --- a/sysdeps/powerpc/Makefile
> > +++ b/sysdeps/powerpc/Makefile
> > @@ -61,11 +61,6 @@ ifeq ($(subdir),misc)
> >   sysdep_headers += sys/platform/ppc.h
> >   tests += test-gettimebase
> >   tests += tst-set_ppr
> > -
> > -# This test is expected to run and exit with EXIT_UNSUPPORTED on
> > -# processors that do not implement the Power ISA 2.06 or greater.
> > -# But the test makes use of instructions from Power ISA 2.06 and 2.07.
> > -CFLAGS-tst-set_ppr.c += -Wa,-many
> >   endif
> > 
> >   ifeq ($(subdir),wcsmbs)
> 
> OK.
> 
> > diff --git a/sysdeps/powerpc/tst-set_ppr.c b/sysdeps/powerpc/tst-set_ppr.c
> > index 7684f5d6ea90..e80da1532063 100644
> > --- a/sysdeps/powerpc/tst-set_ppr.c
> > +++ b/sysdeps/powerpc/tst-set_ppr.c
> > @@ -44,7 +44,8 @@ get_thread_priority (void)
> >   {
> >     /* Read the PPR.  */
> >     ppr_t ppr;
> > -  asm volatile (MFPPR" %0" : "=r"(ppr));
> > +  asm volatile (".machine push; .machine power7; "MFPPR" %0; .machine pop"
> 
> Does it matter if power7 is quoted? LGTM to me otherwise.

Unquoted works.

This is not documented consistently in the binutils "Using as" page.
In 9.36.2 "PowerPC Assembler Directives", it calls the text after
".machine" a "string ... enclosed in double quotes". However, it
continues with examples for "push" and "pop" strongly suggesting
those should also be in quotes.

But, this is not consistent in current glibc code, with many examples
of quoted and unquoted machine strings and push/pop.

In 9.41.4 "Assembler Directives" (under 9.41 "IBM S/390 Dependent
Features"), it says "the cpu string has to be put in double quotes
in case it contains characters not appropriate for identifiers".
(So, does it actually need to be in quotes if it _doesn't_ contain
such characters?)

I could go either way if there's a strong opinion. It works as-is.

Thanks for the review!

PC
diff mbox series

Patch

diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
index 09860ffc0155..5e6cb07ce66d 100644
--- a/sysdeps/powerpc/Makefile
+++ b/sysdeps/powerpc/Makefile
@@ -61,11 +61,6 @@  ifeq ($(subdir),misc)
 sysdep_headers += sys/platform/ppc.h
 tests += test-gettimebase
 tests += tst-set_ppr
-
-# This test is expected to run and exit with EXIT_UNSUPPORTED on
-# processors that do not implement the Power ISA 2.06 or greater.
-# But the test makes use of instructions from Power ISA 2.06 and 2.07.
-CFLAGS-tst-set_ppr.c += -Wa,-many
 endif
 
 ifeq ($(subdir),wcsmbs)
diff --git a/sysdeps/powerpc/tst-set_ppr.c b/sysdeps/powerpc/tst-set_ppr.c
index 7684f5d6ea90..e80da1532063 100644
--- a/sysdeps/powerpc/tst-set_ppr.c
+++ b/sysdeps/powerpc/tst-set_ppr.c
@@ -44,7 +44,8 @@  get_thread_priority (void)
 {
   /* Read the PPR.  */
   ppr_t ppr;
-  asm volatile (MFPPR" %0" : "=r"(ppr));
+  asm volatile (".machine push; .machine power7; "MFPPR" %0; .machine pop"
+		: "=r"(ppr));
   /* Return the thread priority value.  */
   return EXTRACT_THREAD_PRIORITY (ppr);
 }