Message ID | 20210925202746.819385-1-pc@us.ibm.com |
---|---|
State | New |
Headers | show |
Series | [powerpc] Fix unrecognized instruction errors with recent binutils | expand |
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.
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 --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); }