regression with binutils 2.28 for ppc

regression with binutils 2.28 for ppc

[Waldemar Brodkorb]

Hi Developers,

since commit b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a I am getting
following error when building a Linux Kernel targeting
qemu-system-ppc (-M macppc).

{standard input}: Assembler messages:
{standard input}:2156: Error: unrecognized opcode: `ptesync'
gmake[8]: *** [scripts/Makefile.build:288: arch/powerpc/lib/sstep.o]
Error 1
gmake[7]: *** [scripts/Makefile.build:550: arch/powerpc/lib] Error 2
gmake[6]: *** [Makefile:1831: arch/powerpc] Error 2
gmake[5]: ***
[/home/wbx/embedded-test/openadk/mk/kernel-build.mk:91:
/home/wbx/embedded-test/openadk/build_qemu-ppc-macppc_uclibc-ng_hard/linux/vmlinux]
Error 2

I am testing with Linux kernel from today's git. So I think nobody
recognized the regression in binutils 2.29, yet.

Anything I can do about it?

best regards
 Waldemar

Re: regression with binutils 2.28 for ppc

[Peter Bergner]

On 2/16/22 7:35 AM, Waldemar Brodkorb wrote:
> since commit b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a I am getting
> following error when building a Linux Kernel targeting
> qemu-system-ppc (-M macppc).
> 
> {standard input}: Assembler messages:
> {standard input}:2156: Error: unrecognized opcode: `ptesync'
> gmake[8]: *** [scripts/Makefile.build:288: arch/powerpc/lib/sstep.o]
> Error 1
> gmake[7]: *** [scripts/Makefile.build:550: arch/powerpc/lib] Error 2
> gmake[6]: *** [Makefile:1831: arch/powerpc] Error 2
> gmake[5]: ***
> [/home/wbx/embedded-test/openadk/mk/kernel-build.mk:91:
> /home/wbx/embedded-test/openadk/build_qemu-ppc-macppc_uclibc-ng_hard/linux/vmlinux]
> Error 2

The ptesync insn is enabled with the PPC64 flag, so should be available for
all 64-bit assembles and disabled for all 32-bit assembles.  What assembler
options are you using?  It's hard to tell whether you are building a
64-bit kernel, in which ptesync should be fine or whether you are building
a 32-bit kernel, in which case, ptesync would be a kernel error (for using it).

Peter

Re: regression with binutils 2.28 for ppc

[Peter Bergner]

On 2/16/22 8:32 AM, Peter Bergner via Binutils wrote:
> On 2/16/22 7:35 AM, Waldemar Brodkorb wrote:
>> since commit b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a I am getting
>> following error when building a Linux Kernel targeting
>> qemu-system-ppc (-M macppc).
>>
>> {standard input}: Assembler messages:
>> {standard input}:2156: Error: unrecognized opcode: `ptesync'

The commit you mention is Alan's fix for sticky bits.
His commit message mentions the change will probably expose
broken .machine usage, so I'm guessing this will end up being
a kernel issue.

Peter

Re: regression with binutils 2.28 for ppc

[Waldemar Brodkorb]

Hi Peter,
Peter Bergner wrote,

> On 2/16/22 8:32 AM, Peter Bergner via Binutils wrote:
> > On 2/16/22 7:35 AM, Waldemar Brodkorb wrote:
> >> since commit b25f942e18d6ecd7ec3e2d2e9930eb4f996c258a I am getting
> >> following error when building a Linux Kernel targeting
> >> qemu-system-ppc (-M macppc).
> >>
> >> {standard input}: Assembler messages:
> >> {standard input}:2156: Error: unrecognized opcode: `ptesync'
> 
> The commit you mention is Alan's fix for sticky bits.
> His commit message mentions the change will probably expose
> broken .machine usage, so I'm guessing this will end up being
> a kernel issue.

Yeah, I mean 2.38 not 2.28.
The gcc command used to compile the problematic file is:

  /home/wbx/embedded-test/openadk/toolchain_qemu-ppc-macppc_uclibc-ng_hard/usr/bin/ppc-openadk-linux-uclibc-gcc
  -Wp,-MMD,arch/powerpc/lib/.sstep.o.d -nostdinc
  -I./arch/powerpc/include -I./arch/powerpc/include/gen
  erated  -I./include -I./arch/powerpc/include/uapi
  -I./arch/powerpc/include/generated/uapi -I./include/uapi
  -I./include/generated/uapi -include
  ./include/linux/compiler-version.h -include
  ./include/linux/kconfig.h
   -include ./include/linux/compiler_types.h -D__KERNEL__ -I
   ./arch/powerpc -fmacro-prefix-map=./= -Wall -Wundef
   -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
   -fno-common -fshort-wchar -fno-PIE -Wer
   ror=implicit-function-declaration -Werror=implicit-int
   -Werror=return-type -Wno-format-security -std=gnu89 -mcpu=powerpc
   -mcpu=powerpc -mbig-endian -m32 -msoft-float -pipe -ffixed-r2
   -mmultiple -mno-readonly-in-s
   data -mcpu=powerpc64 -mno-altivec -mno-vsx
   -fno-asynchronous-unwind-tables -mno-string -Wa,-maltivec
   -mbig-endian -fno-delete-null-pointer-checks -Wno-frame-address
   -Wno-format-truncation -Wno-format-overflow -Wn
   o-address-of-packed-member -Os -fno-allow-store-data-races
   -Wframe-larger-than=1024 -fno-stack-protector
   -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable
   -Wno-unused-const-variable -fomit-frame-poi
   nter -fno-stack-clash-protection -Wdeclaration-after-statement
   -Wvla -Wno-pointer-sign -Wcast-function-type
   -Wno-stringop-truncation -Wno-zero-length-bounds
   -Wno-array-bounds -Wno-stringop-overflow -Wno-restrict 
   -Wno-maybe-uninitialized -Wno-alloc-size-larger-than
   -fno-strict-overflow -fno-stack-check -fconserve-stack
   -Werror=date-time -Werror=incompatible-pointer-types
   -Werror=designated-init -Wno-packed-not-aligned -We
   rror    -DKBUILD_MODFILE='"arch/powerpc/lib/sstep"'
   -DKBUILD_BASENAME='"sstep"' -DKBUILD_MODNAME='"sstep"'
   -D__KBUILD_MODNAME=kmod_sstep -c -o arch/powerpc/lib/sstep.o
   arch/powerpc/lib/sstep.c 

It works fine with binutils 2.37.

   best regards
    Waldemar

Re: regression with binutils 2.28 for ppc

[Peter Bergner]

On 2/17/22 3:28 AM, Waldemar Brodkorb wrote:
> -mcpu=powerpc -mbig-endian -m32 -msoft-float -mcpu=powerpc64 -mno-altivec -mno-vsx

So a 32-bit compile, which is why the assembler complains about ptesync.
You could try adding -Wa,-mppc64 or -Wa,-many to your options as a work around.
Newish GCCs removed passing -many to the assembler and the commit you
mentioned changed handling of sticky cpu options which is exposing the
issue.  

The use of those ptesyncs in the kernel really needs to be audited though!
If they are legitimate, then the inline assembler needs to wrap their
use with ".machine push ; .machine ppc64 ; ptesync ; .machine pop".

Peter

Re: regression with binutils 2.28 for ppc

[Alan Modra]

On Thu, Feb 17, 2022 at 02:03:24PM -0600, Peter Bergner wrote:
> On 2/17/22 3:28 AM, Waldemar Brodkorb wrote:
> > -mcpu=powerpc -mbig-endian -m32 -msoft-float -mcpu=powerpc64 -mno-altivec -mno-vsx
> 
> So a 32-bit compile, which is why the assembler complains about ptesync.

Not from what you posted there.  The last -mcpu=powerpc64 ought to
enable ppc64.  If you remove that, then yes, you'll get a complaint
about ptesync.

> You could try adding -Wa,-mppc64 or -Wa,-many to your options as a work around.
> Newish GCCs removed passing -many to the assembler and the commit you
> mentioned changed handling of sticky cpu options which is exposing the
> issue.  

-Wa cpu hacks won't work any more.  gcc emits a .machine directive
early in the assembly output that will override them.  I think that
early .machine is just plain wrong.  .machine ought to only be emitted
by gcc to handle something like a pragma or attribute changing code
generation.  That's the only compromise I can see to satisfy the
disparate users of the toolchain.  On the one hand you have developers
who'd like an error when they accidentally use the wrong assembly (and
gcc developers who'd rather not find out gcc is emitting invalid
instructions by user reports of crashes!), and on the other hand you
have kernel delelopers who need to write code supporting a whole range
of cpus who just want the assembler to accept anything.

> The use of those ptesyncs in the kernel really needs to be audited though!
> If they are legitimate, then the inline assembler needs to wrap their
> use with ".machine push ; .machine ppc64 ; ptesync ; .machine pop".

Right.  Or we should allow the user command line to control the
assembler, even with -Wa,-many if they so desire.  But that's killed
by that stupid .machine from gcc.

I didn't even get as far as Waldemar.  With recent gcc, binutils and
linux git source my
ARCH=powerpc CROSS_COMPILE=powerpc-linux- pmac32_defconfig
kernel compile bombs here:

powerpc-linux-gcc -Wp,-MMD,arch/powerpc/kernel/ptrace/.ptrace.o.d -nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated  -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -I ./arch/powerpc -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu89 -mcpu=powerpc -mcpu=powerpc -mbig-endian -m32 -msoft-float -pipe -ffixed-r2 -mmultiple -mno-readonly-in-sdata -mcpu=powerpc64 -mno-altivec -mno-vsx -fno-asynchronous-unwind-tables -mno-string -Wa,-maltivec -mbig-endian -mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -fno-delete-null-pointer-checks -Wno-frame-address -Wno-format-truncation -Wno-format-overflow -Wno-address-of-packed-member -O2 -fno-allow-store-data-races -Wframe-larger-than=1024 -fstack-protector-strong -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -ftrivial-auto-var-init=zero -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wno-stringop-truncation -Wno-zero-length-bounds -Wno-array-bounds -Wno-stringop-overflow -Wno-restrict -Wno-maybe-uninitialized -Wno-alloc-size-larger-than -fno-strict-overflow -fno-stack-check -fconserve-stack -Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init -Wno-packed-not-aligned -mstack-protector-guard-offset=576 -Werror    -DKBUILD_MODFILE='"arch/powerpc/kernel/ptrace/ptrace"' -DKBUILD_BASENAME='"ptrace"' -DKBUILD_MODNAME='"ptrace"' -D__KBUILD_MODNAME=kmod_ptrace -c -o arch/powerpc/kernel/ptrace/ptrace.o arch/powerpc/kernel/ptrace/ptrace.c 
{standard input}: Assembler messages:
{standard input}:997: Error: unrecognized opcode: `mfsrin'
{standard input}:1003: Error: unrecognized opcode: `mtsrin'
{standard input}:1028: Error: unrecognized opcode: `mfsrin'
{standard input}:1034: Error: unrecognized opcode: `mtsrin'
{standard input}:1061: Error: unrecognized opcode: `mfsrin'
{standard input}:1067: Error: unrecognized opcode: `mtsrin'
make[3]: *** [scripts/Makefile.build:288: arch/powerpc/kernel/ptrace/ptrace.o] Error 1

So, last -mcpu is powerpc64.  gas quite correctly flags an error on
instructions phased out for ppc64.

-- 
Alan Modra
Australia Development Lab, IBM

Re: regression with binutils 2.28 for ppc

[Peter Bergner]

On 2/17/22 7:34 PM, Alan Modra wrote:
> On Thu, Feb 17, 2022 at 02:03:24PM -0600, Peter Bergner wrote:
>> On 2/17/22 3:28 AM, Waldemar Brodkorb wrote:
>>> -mcpu=powerpc -mbig-endian -m32 -msoft-float -mcpu=powerpc64 -mno-altivec -mno-vsx
>>
>> So a 32-bit compile, which is why the assembler complains about ptesync.
> 
> Not from what you posted there.  The last -mcpu=powerpc64 ought to
> enable ppc64.  If you remove that, then yes, you'll get a complaint
> about ptesync.

Well it's a 32-bit compile in that the obj is Elf32:

linux$ gcc -m32 -mcpu=powerpc64 simple.c 
linux$ file a.out 
a.out: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, BuildID[sha1]=804e1b0621e258ef9a1e4691d5f0764e4c5e852a, not stripped

That said, the -mcpu=power64 with newish gccs seems to emit a .machine ppc64.
The older gcc I was using seems to emit a ".machine ppc" which is why
I saw the same issue with ptesync when using a new binutils.


>> You could try adding -Wa,-mppc64 or -Wa,-many to your options as a work around.
>> Newish GCCs removed passing -many to the assembler and the commit you
>> mentioned changed handling of sticky cpu options which is exposing the
>> issue.  
> 
> -Wa cpu hacks won't work any more.

Ah right, thanks for correcting me!


>> The use of those ptesyncs in the kernel really needs to be audited though!
>> If they are legitimate, then the inline assembler needs to wrap their
>> use with ".machine push ; .machine ppc64 ; ptesync ; .machine pop".
> 
> Right.  Or we should allow the user command line to control the
> assembler, even with -Wa,-many if they so desire.  But that's killed
> by that stupid .machine from gcc.

I thought we were moving towards more reliance on .machine and not away
from it?  You think we shouldn't be?




> I didn't even get as far as Waldemar.  With recent gcc, binutils and
> linux git source my
> ARCH=powerpc CROSS_COMPILE=powerpc-linux- pmac32_defconfig
> kernel compile bombs here:
> 
[snip]
> {standard input}:997: Error: unrecognized opcode: `mfsrin'
[snip]
> So, last -mcpu is powerpc64.  gas quite correctly flags an error on
> instructions phased out for ppc64.

This is probably the difference between new gccs emiting .machine ppc64
when using -mcpu=powerpc64 and old gccs that emit .machine ppc.
One more confusing thing to have to handle! :-(


Given all the above though, I'm surprised the kernel team hasn't hit
this already and complained to us about it! :-) 


Peter

Re: regression with binutils 2.28 for ppc

[Alan Modra]

On Thu, Feb 17, 2022 at 10:21:30PM -0600, Peter Bergner wrote:
> On 2/17/22 7:34 PM, Alan Modra wrote:
> > On Thu, Feb 17, 2022 at 02:03:24PM -0600, Peter Bergner wrote:
> >> The use of those ptesyncs in the kernel really needs to be audited though!
> >> If they are legitimate, then the inline assembler needs to wrap their
> >> use with ".machine push ; .machine ppc64 ; ptesync ; .machine pop".
> > 
> > Right.  Or we should allow the user command line to control the
> > assembler, even with -Wa,-many if they so desire.  But that's killed
> > by that stupid .machine from gcc.
> 
> I thought we were moving towards more reliance on .machine and not away
> from it?  You think we shouldn't be?

I thought it wasn't a great idea when we started using it in 2015 or
so.  You can probably find archived email of me saying that.  ;-)
Nothing has changed since then to make me think that gcc controlling
the assembler by both command-line options and a source directive at
the start of assembly is a good idea.

I'm tempted to hack gas to ignore the first .machine from gcc, if no
code has been emitted and the .machine is a subset of what is given by
the command line.

> This is probably the difference between new gccs emiting .machine ppc64
> when using -mcpu=powerpc64 and old gccs that emit .machine ppc.

Ah, gcc pr101393 (which was about 403, but same thing, .machine ppc
rather than the correct machine).

> Given all the above though, I'm surprised the kernel team hasn't hit
> this already and complained to us about it! :-) 

I guess the focus has been on 64-bit kernels.

-- 
Alan Modra
Australia Development Lab, IBM

Re: regression with binutils 2.28 for ppc

[Alan Modra]

On Sun, Feb 20, 2022 at 10:28:06PM +1030, Alan Modra wrote:
> On Thu, Feb 17, 2022 at 10:21:30PM -0600, Peter Bergner wrote:
> > On 2/17/22 7:34 PM, Alan Modra wrote:
> > > On Thu, Feb 17, 2022 at 02:03:24PM -0600, Peter Bergner wrote:
> > >> The use of those ptesyncs in the kernel really needs to be audited though!
> > >> If they are legitimate, then the inline assembler needs to wrap their
> > >> use with ".machine push ; .machine ppc64 ; ptesync ; .machine pop".
> > > 
> > > Right.  Or we should allow the user command line to control the
> > > assembler, even with -Wa,-many if they so desire.  But that's killed
> > > by that stupid .machine from gcc.
> > 
> > I thought we were moving towards more reliance on .machine and not away
> > from it?  You think we shouldn't be?
> 
> I thought it wasn't a great idea when we started using it in 2015 or
> so.  You can probably find archived email of me saying that.  ;-)
> Nothing has changed since then to make me think that gcc controlling
> the assembler by both command-line options and a source directive at
> the start of assembly is a good idea.
> 
> I'm tempted to hack gas to ignore the first .machine from gcc, if no
> code has been emitted and the .machine is a subset of what is given by
> the command line.

That didn't work, because gcc pr59828 isn't fixed and what is on the
gas command line for the kernel compile is incorrect.  The .machine is
*not* a subset of the command line options.  I'm not going to try more
complicated hacks in gas trying to deduce what gcc really should have
passed on the command line.

> > This is probably the difference between new gccs emiting .machine ppc64
> > when using -mcpu=powerpc64 and old gccs that emit .machine ppc.
> 
> Ah, gcc pr101393 (which was about 403, but same thing, .machine ppc
> rather than the correct machine).

All things considered, I think all I can do in gas is to partially
revert commit b25f942e18d6 which made .machine more strict.  At least
that way -many (passed by release gcc) or other user sticky -Wa
options like -maltivec will not be lost.  Bad luck if a user wants
-mcpu=power9 -Wa,-power10 for example.

	* config/tc-ppc.c (ppc_machine): Treat an early .machine specially,
	keeping sticky options to work around gcc bugs.

diff --git a/gas/config/tc-ppc.c b/gas/config/tc-ppc.c
index 054f9c72161..89bc7d3f9b9 100644
--- a/gas/config/tc-ppc.c
+++ b/gas/config/tc-ppc.c
@@ -5965,7 +5965,30 @@ ppc_machine (int ignore ATTRIBUTE_UNUSED)
 	     options do not count as a new machine, instead they add
 	     to currently selected opcodes.  */
 	  ppc_cpu_t machine_sticky = 0;
-	  new_cpu = ppc_parse_cpu (ppc_cpu, &machine_sticky, cpu_string);
+	  /* Unfortunately, some versions of gcc emit a .machine
+	     directive very near the start of the compiler's assembly
+	     output file.  This is bad because it overrides user -Wa
+	     cpu selection.  Worse, there are versions of gcc that
+	     emit the *wrong* cpu, not even respecting the -mcpu given
+	     to gcc.  See gcc pr101393.  And to compound the problem,
+	     as of 20220222 gcc doesn't pass the correct cpu option to
+	     gas on the command line.  See gcc pr59828.  Hack around
+	     this by keeping sticky options for an early .machine.  */
+	  asection *sec;
+	  for (sec = stdoutput->sections; sec != NULL; sec = sec->next)
+	    {
+	      segment_info_type *info = seg_info (sec);
+	      /* Are the frags for this section perturbed from their
+		 initial state?  Even .align will count here.  */
+	      if (info != NULL
+		  && (info->frchainP->frch_root != info->frchainP->frch_last
+		      || info->frchainP->frch_root->fr_type != rs_fill
+		      || info->frchainP->frch_root->fr_fix != 0))
+		break;
+	    }
+	  new_cpu = ppc_parse_cpu (ppc_cpu,
+				   sec == NULL ? &sticky : &machine_sticky,
+				   cpu_string);
 	  if (new_cpu != 0)
 	    ppc_cpu = new_cpu;
 	  else

-- 
Alan Modra
Australia Development Lab, IBM

Re: regression with binutils 2.28 for ppc

[Waldemar Brodkorb]

Hi Alan,
Alan Modra wrote,

> All things considered, I think all I can do in gas is to partially
> revert commit b25f942e18d6 which made .machine more strict.  At least
> that way -many (passed by release gcc) or other user sticky -Wa
> options like -maltivec will not be lost.  Bad luck if a user wants
> -mcpu=power9 -Wa,-power10 for example.
> 
> 	* config/tc-ppc.c (ppc_machine): Treat an early .machine specially,
> 	keeping sticky options to work around gcc bugs.
> 
> diff --git a/gas/config/tc-ppc.c b/gas/config/tc-ppc.c
> index 054f9c72161..89bc7d3f9b9 100644
> --- a/gas/config/tc-ppc.c
> +++ b/gas/config/tc-ppc.c
> @@ -5965,7 +5965,30 @@ ppc_machine (int ignore ATTRIBUTE_UNUSED)
>  	     options do not count as a new machine, instead they add
>  	     to currently selected opcodes.  */
>  	  ppc_cpu_t machine_sticky = 0;
> -	  new_cpu = ppc_parse_cpu (ppc_cpu, &machine_sticky, cpu_string);
> +	  /* Unfortunately, some versions of gcc emit a .machine
> +	     directive very near the start of the compiler's assembly
> +	     output file.  This is bad because it overrides user -Wa
> +	     cpu selection.  Worse, there are versions of gcc that
> +	     emit the *wrong* cpu, not even respecting the -mcpu given
> +	     to gcc.  See gcc pr101393.  And to compound the problem,
> +	     as of 20220222 gcc doesn't pass the correct cpu option to
> +	     gas on the command line.  See gcc pr59828.  Hack around
> +	     this by keeping sticky options for an early .machine.  */
> +	  asection *sec;
> +	  for (sec = stdoutput->sections; sec != NULL; sec = sec->next)
> +	    {
> +	      segment_info_type *info = seg_info (sec);
> +	      /* Are the frags for this section perturbed from their
> +		 initial state?  Even .align will count here.  */
> +	      if (info != NULL
> +		  && (info->frchainP->frch_root != info->frchainP->frch_last
> +		      || info->frchainP->frch_root->fr_type != rs_fill
> +		      || info->frchainP->frch_root->fr_fix != 0))
> +		break;
> +	    }
> +	  new_cpu = ppc_parse_cpu (ppc_cpu,
> +				   sec == NULL ? &sticky : &machine_sticky,
> +				   cpu_string);
>  	  if (new_cpu != 0)
>  	    ppc_cpu = new_cpu;
>  	  else

That fixes the kernel compile for me, too. thanks Alan!

best regards
 Waldemar