[PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Palmer Dabbelt]

This has some cascading fallout related to PC-relative references to
SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
binutils [2], but GP isn't useful in PIC so we might as well just stop
referencing it at all.

Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
Link: https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

---

I haven't tested thiis or the binutils patch.  There's a handful of
coupled issues here that might take a bit to untangle, but this came up
in the RISC-V LLVM sync this morning so I figured it would be best to
send something along.
---
 sysdeps/riscv/start.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
index 6dfe65273f..5eaa8ccf2d 100644
--- a/sysdeps/riscv/start.S
+++ b/sysdeps/riscv/start.S
@@ -71,7 +71,9 @@ END (ENTRY_POINT)
 load_gp:
 .option push
 .option norelax
+#if !(defined(__PIC__) || defined(__pic__) || defined(PIC) || defined(pic))
 	lla   gp, __global_pointer$
+#endif
 .option pop
 	ret
 
-- 
2.40.1

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Palmer Dabbelt]

On Thu, 08 Jun 2023 09:44:37 PDT (-0700), Palmer Dabbelt wrote:
> This has some cascading fallout related to PC-relative references to
> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
> binutils [2], but GP isn't useful in PIC so we might as well just stop
> referencing it at all.
>
> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
> Link: https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
>
> I haven't tested thiis or the binutils patch.  There's a handful of
> coupled issues here that might take a bit to untangle, but this came up
> in the RISC-V LLVM sync this morning so I figured it would be best to
> send something along.

Nelson found the binutils patch to allow this behavior 890744e8585 
("RISC-V: PR28789, Reject R_RISCV_PCREL relocations with ABS symbol in 
PIC/PIE."), it was committed with the bit that allows these symbols to 
be resolved when linking.  Dropping that causes pretty much everything 
to fail to link, so at least that part of the mystery is solved.

That leaves us with what to do here.  Nelson pointed out that just 
leaving GP uninitialized is probably the wrong way to go, so at least 
setting it to 0 seems reasonable.  It also might be better to fix the 
emulparams bug and just keep putting GP in the CRTs, though, as in 
theory this is usable for PIEs and thus removing it is an ABI break 
(though as far as I know we're not using it and it's currently subtly 
broken).

The rough plan would be to get a patch in binutils that makes the 
SHN_ABS/PIC resolution optional (probably both a command-line argument 
and a configure-time default) and then try a ditro rebuild with it to 
see if anything else breaks.  With the binutils release coming up we're 
probably not going to have time to get confidence in any changes over 
there, so it's probably best to wait a bit on this one as well just to 
avoid risking breaking anything.

> ---
>  sysdeps/riscv/start.S | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
> index 6dfe65273f..5eaa8ccf2d 100644
> --- a/sysdeps/riscv/start.S
> +++ b/sysdeps/riscv/start.S
> @@ -71,7 +71,9 @@ END (ENTRY_POINT)
>  load_gp:
>  .option push
>  .option norelax
> +#if !(defined(__PIC__) || defined(__pic__) || defined(PIC) || defined(pic))
>  	lla   gp, __global_pointer$
> +#endif
>  .option pop
>  	ret

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Jeff Law]

On 6/26/23 07:44, Palmer Dabbelt wrote:
> On Thu, 08 Jun 2023 09:44:37 PDT (-0700), Palmer Dabbelt wrote:
>> This has some cascading fallout related to PC-relative references to
>> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
>> binutils [2], but GP isn't useful in PIC so we might as well just stop
>> referencing it at all.
>>
>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
>> Link: 
>> https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>>
>> I haven't tested thiis or the binutils patch.  There's a handful of
>> coupled issues here that might take a bit to untangle, but this came up
>> in the RISC-V LLVM sync this morning so I figured it would be best to
>> send something along.
> 
> Nelson found the binutils patch to allow this behavior 890744e8585 
> ("RISC-V: PR28789, Reject R_RISCV_PCREL relocations with ABS symbol in 
> PIC/PIE."), it was committed with the bit that allows these symbols to 
> be resolved when linking.  Dropping that causes pretty much everything 
> to fail to link, so at least that part of the mystery is solved.
> 
> That leaves us with what to do here.  Nelson pointed out that just 
> leaving GP uninitialized is probably the wrong way to go, so at least 
> setting it to 0 seems reasonable.  It also might be better to fix the 
> emulparams bug and just keep putting GP in the CRTs, though, as in 
> theory this is usable for PIEs and thus removing it is an ABI break 
> (though as far as I know we're not using it and it's currently subtly 
> broken).
> 
> The rough plan would be to get a patch in binutils that makes the 
> SHN_ABS/PIC resolution optional (probably both a command-line argument 
> and a configure-time default) and then try a ditro rebuild with it to 
> see if anything else breaks.  With the binutils release coming up we're 
> probably not going to have time to get confidence in any changes over 
> there, so it's probably best to wait a bit on this one as well just to 
> avoid risking breaking anything.
So just from a staging standpoint.  The fall Fedora releases often do 
not do full distro rebuilds, but the spring releases always perform full 
Fedora rebuilds.  Additionally there should be another release of 
binutils & glibc in time for the spring 2024 Fedora build.

Meaning that there's no huge rush to push this through, at least from a 
Fedora standpoint -- the real goal should be to have all the pieces in 
place for the spring 2024 release.  Meaning binutils-2.42 and glibc-2.39.

I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do 
think they do full builds in the spring as well (mostly to pick up the 
new compilers).  So further support for trying to line things up fo the 
spring 2024 releases.

jeff

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[David Abdurachmanov]

On Mon, Jun 26, 2023 at 4:56 PM Jeff Law <jlaw@ventanamicro.com> wrote:
>
>
>
> On 6/26/23 07:44, Palmer Dabbelt wrote:
> > On Thu, 08 Jun 2023 09:44:37 PDT (-0700), Palmer Dabbelt wrote:
> >> This has some cascading fallout related to PC-relative references to
> >> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
> >> binutils [2], but GP isn't useful in PIC so we might as well just stop
> >> referencing it at all.
> >>
> >> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
> >> Link:
> >> https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >>
> >> ---
> >>
> >> I haven't tested thiis or the binutils patch.  There's a handful of
> >> coupled issues here that might take a bit to untangle, but this came up
> >> in the RISC-V LLVM sync this morning so I figured it would be best to
> >> send something along.
> >
> > Nelson found the binutils patch to allow this behavior 890744e8585
> > ("RISC-V: PR28789, Reject R_RISCV_PCREL relocations with ABS symbol in
> > PIC/PIE."), it was committed with the bit that allows these symbols to
> > be resolved when linking.  Dropping that causes pretty much everything
> > to fail to link, so at least that part of the mystery is solved.
> >
> > That leaves us with what to do here.  Nelson pointed out that just
> > leaving GP uninitialized is probably the wrong way to go, so at least
> > setting it to 0 seems reasonable.  It also might be better to fix the
> > emulparams bug and just keep putting GP in the CRTs, though, as in
> > theory this is usable for PIEs and thus removing it is an ABI break
> > (though as far as I know we're not using it and it's currently subtly
> > broken).
> >
> > The rough plan would be to get a patch in binutils that makes the
> > SHN_ABS/PIC resolution optional (probably both a command-line argument
> > and a configure-time default) and then try a ditro rebuild with it to
> > see if anything else breaks.  With the binutils release coming up we're
> > probably not going to have time to get confidence in any changes over
> > there, so it's probably best to wait a bit on this one as well just to
> > avoid risking breaking anything.
> So just from a staging standpoint.  The fall Fedora releases often do
> not do full distro rebuilds, but the spring releases always perform full
> Fedora rebuilds.  Additionally there should be another release of
> binutils & glibc in time for the spring 2024 Fedora build.

The mass rebuilds are done for every Fedora release. Just to double
check I looked at release detailed schedules from F30-39. All had/have
mass rebuild scheduled.

>
> Meaning that there's no huge rush to push this through, at least from a
> Fedora standpoint -- the real goal should be to have all the pieces in
> place for the spring 2024 release.  Meaning binutils-2.42 and glibc-2.39.

For the last several Fedora releases binutils lagged by one version in
Fedora (official). Fedora 40 (spring time) will get 2.41.

We break that rule in Fedora/RISCV land, and we run the latest
released binutils version (as soon as Nick lands it in Rawhide).

david

>
> I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do
> think they do full builds in the spring as well (mostly to pick up the
> new compilers).  So further support for trying to line things up fo the
> spring 2024 releases.
>
> jeff

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Jeff Law]

On 6/26/23 08:15, David Abdurachmanov wrote:

> 
> The mass rebuilds are done for every Fedora release. Just to double
> check I looked at release detailed schedules from F30-39. All had/have
> mass rebuild scheduled.
?!?  That certainly wasn't my recollection.  The consistent pattern was 
to try and avoid that fall mass rebuild and instead only rebuild 
packages which actually changed.

> 
> For the last several Fedora releases binutils lagged by one version in
> Fedora (official). Fedora 40 (spring time) will get 2.41.
Yea, that has been a bit of a pattern.  Though it was driven more by not 
seeing much benefit from moving to the latest bits, but seeing certain 
risks that we weren't ready to take on.

Jeff

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Andreas Schwab]

On Jun 26 2023, Jeff Law wrote:

> I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do
> think they do full builds in the spring as well (mostly to pick up the new
> compilers).

I do not trigger full rebuilds for Tumbleweed, as that would take weeks,
if not months to finish.  It's already hard enough to catch up with the
normal updates.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Florian Weimer]

* Jeff Law:

> So just from a staging standpoint.  The fall Fedora releases often do
> not do full distro rebuilds, but the spring releases always perform
> full Fedora rebuilds.  Additionally there should be another release of
> binutils & glibc in time for the spring 2024 Fedora build.

Fedora doesn't have a real RISC-V port yet.  If that changes, it implies
a mass rebuild on official infrasturcutr.

> I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do
> think they do full builds in the spring as well (mostly to pick up the
> new compilers).  So further support for trying to line things up fo
> the spring 2024 releases.

Debian doesn't do mass rebuilds.

But as far as I understand it, this change does not need a mass rebuild,
it's just necessary to rebuild glibc for the new crt files before the
binutils update.

Thanks,
Florian

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Palmer Dabbelt]

On Mon, 26 Jun 2023 07:49:03 PDT (-0700), fweimer@redhat.com wrote:
> * Jeff Law:
>
>> So just from a staging standpoint.  The fall Fedora releases often do
>> not do full distro rebuilds, but the spring releases always perform
>> full Fedora rebuilds.  Additionally there should be another release of
>> binutils & glibc in time for the spring 2024 Fedora build.
>
> Fedora doesn't have a real RISC-V port yet.  If that changes, it implies
> a mass rebuild on official infrasturcutr.
>
>> I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do
>> think they do full builds in the spring as well (mostly to pick up the
>> new compilers).  So further support for trying to line things up fo
>> the spring 2024 releases.
>
> Debian doesn't do mass rebuilds.
>
> But as far as I understand it, this change does not need a mass rebuild,
> it's just necessary to rebuild glibc for the new crt files before the
> binutils update.

Sorry I wasn't clear: the mass rebuild would be necessary to see if 
anything else is depending on the binutils bug that currently allows 
PC-relative references to symbols in SHN_ABS when linked PIE/PIC.  We do 
these test runs sometimes for these wide-reaching bug fixes, just to try 
and see if anyone depends on the bug.  Sometimes it's just a 
buildroot-type build, but sometimes I ask David to do it in the 
out-of-tree Fedora port as IIUC that gets somewhat regular rebuilds as 
it is.

In this case we don't need to do the rebuild to know there's at least 
one dependency on the binutils bug.  This patch patch is one way of 
removing the references (though as discussed above, this probably isn't 
the right way to do it).  There might be other bits of software that 
depend on the bug, though, and we won't find those until we try and 
rebuild them.

We'll still need the standard compatibility song and dance, as having 
just one dependency on the bug is sufficient to require that work.  
It'll just be faster to get all the bug dependencies fixed if we start 
now, rather than waiting a few years to propose changing the default 
binutils behavior and finding stuff still breaks.

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Palmer Dabbelt]

On Mon, 26 Jun 2023 06:55:54 PDT (-0700), Jeff Law wrote:
>
>
> On 6/26/23 07:44, Palmer Dabbelt wrote:
>> On Thu, 08 Jun 2023 09:44:37 PDT (-0700), Palmer Dabbelt wrote:
>>> This has some cascading fallout related to PC-relative references to
>>> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
>>> binutils [2], but GP isn't useful in PIC so we might as well just stop
>>> referencing it at all.
>>>
>>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
>>> Link:
>>> https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
>>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>>
>>> ---
>>>
>>> I haven't tested thiis or the binutils patch.  There's a handful of
>>> coupled issues here that might take a bit to untangle, but this came up
>>> in the RISC-V LLVM sync this morning so I figured it would be best to
>>> send something along.
>>
>> Nelson found the binutils patch to allow this behavior 890744e8585
>> ("RISC-V: PR28789, Reject R_RISCV_PCREL relocations with ABS symbol in
>> PIC/PIE."), it was committed with the bit that allows these symbols to
>> be resolved when linking.  Dropping that causes pretty much everything
>> to fail to link, so at least that part of the mystery is solved.
>>
>> That leaves us with what to do here.  Nelson pointed out that just
>> leaving GP uninitialized is probably the wrong way to go, so at least
>> setting it to 0 seems reasonable.  It also might be better to fix the
>> emulparams bug and just keep putting GP in the CRTs, though, as in
>> theory this is usable for PIEs and thus removing it is an ABI break
>> (though as far as I know we're not using it and it's currently subtly
>> broken).
>>
>> The rough plan would be to get a patch in binutils that makes the
>> SHN_ABS/PIC resolution optional (probably both a command-line argument
>> and a configure-time default) and then try a ditro rebuild with it to
>> see if anything else breaks.  With the binutils release coming up we're
>> probably not going to have time to get confidence in any changes over
>> there, so it's probably best to wait a bit on this one as well just to
>> avoid risking breaking anything.
> So just from a staging standpoint.  The fall Fedora releases often do
> not do full distro rebuilds, but the spring releases always perform full
> Fedora rebuilds.  Additionally there should be another release of
> binutils & glibc in time for the spring 2024 Fedora build.
>
> Meaning that there's no huge rush to push this through, at least from a
> Fedora standpoint -- the real goal should be to have all the pieces in
> place for the spring 2024 release.  Meaning binutils-2.42 and glibc-2.39.

Just to be clear: this isn't for the GP-free ABI, it's just a bug fix 
for the GP-enabled ABI.  It happens the bug fix ends up triggering a 
rabbit hole of other bugs, hence the complexity.

> I'm less familiar with the Debian, SuSE and Ubuntu schedules, but I do
> think they do full builds in the spring as well (mostly to pick up the
> new compilers).  So further support for trying to line things up fo the
> spring 2024 releases.
>
> jeff

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Fangrui Song]

On Thu, Jun 8, 2023 at 9:45 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> This has some cascading fallout related to PC-relative references to
> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
> binutils [2], but GP isn't useful in PIC so we might as well just stop
> referencing it at all.
>
> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
> Link: https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
>
> I haven't tested thiis or the binutils patch.  There's a handful of
> coupled issues here that might take a bit to untangle, but this came up
> in the RISC-V LLVM sync this morning so I figured it would be best to
> send something along.
> ---
>  sysdeps/riscv/start.S | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
> index 6dfe65273f..5eaa8ccf2d 100644
> --- a/sysdeps/riscv/start.S
> +++ b/sysdeps/riscv/start.S
> @@ -71,7 +71,9 @@ END (ENTRY_POINT)
>  load_gp:
>  .option push
>  .option norelax
> +#if !(defined(__PIC__) || defined(__pic__) || defined(PIC) || defined(pic))
>         lla   gp, __global_pointer$
> +#endif
>  .option pop
>         ret
>
> --
> 2.40.1
>

I haven't read all the discussion yet.. If we want to detect
-fpic/-fPIC (no distinction in LLVM for non-sparc-non-ppc32 targets).
Checking just __PIC__ is sufficient. __PIC__ seems more popular than
__pic__ for open-source software.

% riscv64-linux-gnu-gcc -fpic -dM -E -xc /dev/null | grep -i pic
#define __pic__ 1
#define __PIC__ 1
#define __riscv_cmodel_pic 1
% clang --target=riscv64 -fpic -dM -E -xc /dev/null | grep -i pic
#define __PIC__ 1
#define __pic__ 1

(-fPIC changes the macro replacements to 2.)

-- 
宋方睿

Re: [PATCH] RISC-V: Stop referencing __global_pointer$ under PIC

[Palmer Dabbelt]

On Mon, 26 Jun 2023 13:12:41 PDT (-0700), maskray@google.com wrote:
> On Thu, Jun 8, 2023 at 9:45 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> This has some cascading fallout related to PC-relative references to
>> SHN_ABS that Jim reported in [1].  I have a workaround for that issue in
>> binutils [2], but GP isn't useful in PIC so we might as well just stop
>> referencing it at all.
>>
>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=24678
>> Link: https://inbox.sourceware.org/binutils/20230608155214.32435-1-palmer@rivosinc.com/T/#u
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>>
>> I haven't tested thiis or the binutils patch.  There's a handful of
>> coupled issues here that might take a bit to untangle, but this came up
>> in the RISC-V LLVM sync this morning so I figured it would be best to
>> send something along.
>> ---
>>  sysdeps/riscv/start.S | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
>> index 6dfe65273f..5eaa8ccf2d 100644
>> --- a/sysdeps/riscv/start.S
>> +++ b/sysdeps/riscv/start.S
>> @@ -71,7 +71,9 @@ END (ENTRY_POINT)
>>  load_gp:
>>  .option push
>>  .option norelax
>> +#if !(defined(__PIC__) || defined(__pic__) || defined(PIC) || defined(pic))
>>         lla   gp, __global_pointer$
>> +#endif
>>  .option pop
>>         ret
>>
>> --
>> 2.40.1
>>
>
> I haven't read all the discussion yet.. If we want to detect
> -fpic/-fPIC (no distinction in LLVM for non-sparc-non-ppc32 targets).
> Checking just __PIC__ is sufficient. __PIC__ seems more popular than
> __pic__ for open-source software.

I found all four used other places in glibc and figured I'd just do the 
same, I'm not really sure it's necessary but it doesn't seem to hurt.

>
> % riscv64-linux-gnu-gcc -fpic -dM -E -xc /dev/null | grep -i pic
> #define __pic__ 1
> #define __PIC__ 1
> #define __riscv_cmodel_pic 1
> % clang --target=riscv64 -fpic -dM -E -xc /dev/null | grep -i pic
> #define __PIC__ 1
> #define __pic__ 1
>
> (-fPIC changes the macro replacements to 2.)
>
> -- 
> 宋方睿