[Bug target/108315] New: -mcpu=power10 changes ABI

[Bug target/108315] New: -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

            Bug ID: 108315
           Summary: -mcpu=power10 changes ABI
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: ABI, wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: amonakov at gcc dot gnu.org
  Target Milestone: ---
            Target: powerpc64le-*-*

Created attachment 54202
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54202&action=edit
testcase

At least the documentation should mention that if intentional.

In the attached example, the function bar is compiled to

bar:
        .localentry     bar,1
        mtctr 3
        mr 12,3
        bctr
        .long 0
        .byte 0,0,0,0,0,0,0,0

i.e. it does not preserve r2 (it's compiled with -mcpu=power10). If the caller
is not compiled with -mcpu=power10, it needs r2 preserved (bar has a
localentry, so the nop in the caller stays a nop after linking).

I verified the testcase misbehaves on Compile Farm's gcc135: as it does not use
any power10-specific instructions, it's runnable there.

[Bug target/108315] -mcpu=power10 changes ABI

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amodra at gcc dot gnu.org

--- Comment #1 from Peter Bergner <bergner at gcc dot gnu.org> ---
CCing Alan, since he probably knows best how this all works, but yes,
-mcpu-power10 changes the ABI, namely it adds pcrel support which is an ABI
extension, where the function compiled with -mcpu=power10 doesn't use r2 to
access its GOT, but uses pcrel code instead.  I think copying over a p10
compiled asm or obj file to a p9 system and them linking there with a non-p10
aware compiler/binutils seems to be a user error on my part.  Just because the
-mcpu=power10 compiled file doesn't use any p10 instructions doesn't mean it's
safe to use on a pre-p10 system.

I'll let Alan correct me if I'm wrong, but this seems like user error to me.

[Bug target/108315] -mcpu=power10 changes ABI

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |linkw at gcc dot gnu.org

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #0)
> Created attachment 54202 [details]
> testcase
> 
> At least the documentation should mention that if intentional.
> 
> In the attached example, the function bar is compiled to
> 
> bar:
>         .localentry     bar,1
>         mtctr 3
>         mr 12,3
>         bctr
>         .long 0
>         .byte 0,0,0,0,0,0,0,0
> 
> i.e. it does not preserve r2 (it's compiled with -mcpu=power10). If the
> caller is not compiled with -mcpu=power10, it needs r2 preserved (bar has a
> localentry, so the nop in the caller stays a nop after linking).

My local 64bit-elfv2-abi spec v1.5 has the following description:

3.4.1. Symbol Values

"The values of these three most significant bits of the st_other field have the
following meanings:

...

1 The local and global entry points are the same, and r2 should be treated as
caller-saved for local and global callers. "

...

"The value of st_other is determined from the .localentry directive as follows:
If the .localentry value is 0, the value of st_other is 0. If the .localentry
value is 1, the value of st_other is 1. Otherwise, the value of st_other is the
logarithm (base 2) of the .localentry value."

The function bar is with st_other value 1, r2 should be treated as
caller-saved, so it doesn't take action to preserve r2.

> 
> I verified the testcase misbehaves on Compile Farm's gcc135: as it does not
> use any power10-specific instructions, it's runnable there.

I tried the attachment on one local machine (also ppc64le p9) and noticed the
linker already did some fix-ups with long_branch.bar stub,

Dump of assembler code for function main:
   0x0000000010000540 <+0>:     lis     r2,4098
   0x0000000010000544 <+4>:     addi    r2,r2,32512
   0x0000000010000548 <+8>:     mflr    r0
   0x000000001000054c <+12>:    nop
   0x0000000010000550 <+16>:    ld      r3,-32728(r2)
   0x0000000010000554 <+20>:    std     r0,16(r1)
   0x0000000010000558 <+24>:    stdu    r1,-32(r1)
   0x000000001000055c <+28>:    bl      0x10000510 <00000038.long_branch.bar>
=> 0x0000000010000560 <+32>:    ld      r2,24(r1)
   0x0000000010000564 <+36>:    addis   r3,r2,-2
   0x0000000010000568 <+40>:    addi    r3,r3,-30328

Dump of assembler code for function 00000038.long_branch.bar:
=> 0x0000000010000510 <+0>:     std     r2,24(r1)
   0x0000000010000514 <+4>:     b       0x10000710 <bar>

which would save r2 onto the corresponding stack slot ahead, it runs well as
expected. Not sure why it doesn't work on your side, maybe this inter-operation
requires some support in newer binutils? My local one is GNU ld 2.34 which is
for final linking (and 2.35 for power10 support, ie. bar.o generation).

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #3 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Alan implemented the special case of .localentry 1 in this patch for the BFD
linker (that appeared in binutils 2.32 if my calculations are correct):
https://sourceware.org/pipermail/binutils/2018-July/103936.html

and, I think, in this patchset for the Gold linker:
https://sourceware.org/pipermail/binutils/2019-July/107486.html

Corresponding support in the LLVM linker (LLD) also seems to appear around
2018.

It looks like de-facto ABI change even if on paper nothing changed, because all
Linux linkers interpreted .localentry 1 accordingly to the general rule and
would silently produce wrong code. Hence my suggestion to document it.

Plus, someone would have to reach out to the mold maintainer, as mold doesn't
seem to implement this special case.

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #4 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Let me address one point separately:

(In reply to Peter Bergner from comment #1)
> CCing Alan, since he probably knows best how this all works, but yes,
> -mcpu-power10 changes the ABI, namely it adds pcrel support which is an ABI
> extension, where the function compiled with -mcpu=power10 doesn't use r2 to
> access its GOT, but uses pcrel code instead.  I think copying over a p10
> compiled asm or obj file to a p9 system and them linking there with a
> non-p10 aware compiler/binutils seems to be a user error on my part.  Just
> because the -mcpu=power10 compiled file doesn't use any p10 instructions
> doesn't mean it's safe to use on a pre-p10 system.
> 
> I'll let Alan correct me if I'm wrong, but this seems like user error to me.

And the user deserves to get a silently mislinked executable that mysteriously
fails at runtime [*] because it's the users' job to tag static libraries with
minimum required ABI level, not the toolchain's. Yeah.

[*] it does not immediately segfault when returning from a localentry 1
function; it just continues with wrong r2, that may very well point to valid
memory

[Bug target/108315] -mcpu=power10 changes ABI

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Alan Modra <amodra at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amodra at gmail dot com

--- Comment #5 from Alan Modra <amodra at gmail dot com> ---
Any caller of bar should be fine.  The localentry value of 1 means it is
advertised as having no requirement on r2 or r12 and r2 is *not* preserved.  A
call that uses R_PPC64_REL24 to bar ought to have an r2 saving stub inserted by
the linker and the nop after the call changed to restore r2.  GNU ld will do
exactly that.

0000000010000540 <00000047.long_branch.bar>:
    10000540:   18 00 41 f8     std     r2,24(r1)
    10000544:   4c 02 00 48     b       10000790 <bar>
        ...

00000000100005a0 <main>:
    100005a0:   02 10 40 3c     lis     r2,4098
    100005a4:   00 7f 42 38     addi    r2,r2,32512
    100005a8:   a6 02 08 7c     mflr    r0
    100005ac:   00 00 00 60     nop
    100005b0:   20 80 62 e8     ld      r3,-32736(r2)
    100005b4:   10 00 01 f8     std     r0,16(r1)
    100005b8:   e1 ff 21 f8     stdu    r1,-32(r1)
    100005bc:   85 ff ff 4b     bl      10000540 <00000047.long_branch.bar>
    100005c0:   18 00 41 e8     ld      r2,24(r1)

There isn't a problem if bar is passed the address of a non-pcrel function in a
shared library as it is in your example.  The call in bar will go to the global
entry point of foo, which will set up r2.  The same thing happens when linking
against foo.o, the function pointer address is that of the global entry of foo.
[amodra@gcc135 p10-interop]$ ./smain 
Hello from foo
Hello, World!

(My dynamic main won't run on gcc135 due to my build machine having a more
recent glibc, but I'm confident it would run if I fixed that.)

I don't see any problem here other than (presumably) missing linker support.

[Bug target/108315] -mcpu=power10 changes ABI

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rui314 at gmail dot com

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
> Plus, someone would have to reach out to the mold maintainer, as mold
> doesn't seem to implement this special case.

@Rui: ^^^

[Bug target/108315] -mcpu=power10 changes ABI

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
This is not a special case at all.

[Bug target/108315] -mcpu=power10 changes ABI

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
To expand a bit: st_other with value 1 was reserved before, and now it
isn't anymore.  Any tool that silently ignores the "special case" of
reserved values will not work correctly (it might sometimes do what is
wanted though).  This is true everywhere always.

[Bug target/108315] -mcpu=power10 changes ABI

[rui314 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #9 from Rui Ueyama <rui314 at gmail dot com> ---
I'm the maintainer of the mold linker. I didn't implement that POWER10 ABI
because I didn't have an access to a POWER10 machine and therefore couldn't
verify the correctness of my implementation.

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #10 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Rui Ueyama from comment #9)
> I'm the maintainer of the mold linker. I didn't implement that POWER10 ABI
> because I didn't have an access to a POWER10 machine and therefore couldn't
> verify the correctness of my implementation.

Thanks for the info. It might be better to explicitly diagnose the '.localentry
1' case and error out instead of producing an executable that will continue to
run with wrong r2 after the mislinked call returns.

[Bug target/108315] -mcpu=power10 changes ABI

[rui314 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #11 from Rui Ueyama <rui314 at gmail dot com> ---
I'll try to add a POWER10 support to mold using Qemu.

[Bug target/108315] -mcpu=power10 changes ABI

[dje at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #12 from David Edelsohn <dje at gcc dot gnu.org> ---
We're working to add a Power10 system to the Compile Farm.  The systems are at
OSUOSL, but Power10 doesn't have official KVM support so the interface to the
OpenStack management system is complicated.

[Bug target/108315] -mcpu=power10 changes ABI

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #13 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #10)
> (In reply to Rui Ueyama from comment #9)
> > I'm the maintainer of the mold linker. I didn't implement that POWER10 ABI
> > because I didn't have an access to a POWER10 machine and therefore couldn't
> > verify the correctness of my implementation.
> 
> Thanks for the info. It might be better to explicitly diagnose the
> '.localentry 1' case and error out instead of producing an executable that
> will continue to run with wrong r2 after the mislinked call returns.

Yes, exactly.  Silently giving any handling to reserved values will never end
well; just a warning would help a lot already (making it non-silent :-) )

FWIW, calling this "POWER10 ABI" is very inexact; it is just part of the ELFv2
ABI (a reserved value in older ABI versions, and with a specific meaning in
newer versions), not only for Power10.  Of course it was created to make code
that uses pcrel addressing faster, but :-)

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #14 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Are you guys really sure you want to blame the user here, considering that all
linkers, including the BFD linker, initially misinterpreted the ABI the same
way?

[Bug target/108315] -mcpu=power10 changes ABI

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #15 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #14)
> Are you guys really sure you want to blame the user here,

I apologise if this hasn't been a nice experience for you.

I'm not blaming anyone, least of all the user.  That is not what bugzilla is
for anyway.  The goal here is to work together on improving the compiler.

I marked the bug as RESOLVED INVALID because a) there is nothing left to be
done to resolve this PR, and b) that is because there never was anything to
be done (in GCC!) in the first place.

If this is not correct, please add some info clarifying that, and reopen the
PR?

> considering that
> all linkers, including the BFD linker, initially misinterpreted the ABI the
> same way?

It wasn't implemented correctly there either, yes.  That does not necessarily
mean the ABI was misinterpreted, but sure, that could be.  In either case
that has nothing to do with GCC.

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |UNCONFIRMED

--- Comment #16 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Well, from the start I was suggesting that GCC documentation for -mcpu=power10
option should inform the user about the problem (regarding linker support).

[Bug target/108315] -mcpu=power10 changes ABI

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #17 from Segher Boessenkool <segher at gcc dot gnu.org> ---
What makes you think we need to tell the user to do something?  There is
nothing that needs to be done as far as I can see?  /confused

[Bug target/108315] -mcpu=power10 changes ABI

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #18 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
It seems you are saying that as long as GCC emits code according to the Holy
Scripture that is the ABI spec, everything is fine. I imagine on other
architectures maintainers are able to consider how the whole toolchain works
together to avoid leaving the user with difficult-to-debug fallout.

[Bug target/108315] -mcpu=power10 changes ABI

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #19 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Rui Ueyama from comment #11)
> I'll try to add a POWER10 support to mold using Qemu.

I noticed some Power10 mold code was committed in March.  Does that mean this
is "fixed" in mold now?  If not, the Power10 GCC farm system is now available
for use.  The Power10 system is: gcc120

[Bug target/108315] -mcpu=power10 changes ABI

[rui314 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #20 from Rui Ueyama <rui314 at gmail dot com> ---
Last time I tried, mold-produced binaries crash on a real POWER10 machine, but
I couldn't debug it due to some issue (gdb's issue or something but I don't
remember exactly what that was.) Let me try again when I have time.

[Bug target/108315] -mcpu=power10 changes ABI

[rui314 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108315

--- Comment #21 from Rui Ueyama <rui314 at gmail dot com> ---
I fixed several issues in mold related to POWER10 compatibility, and all its
unit tests pass on gcc120! I also confirmed that mold can now bootstrap itself
with `-mcpu=power10`. So I believe it's now usable on POWER10 machines.