| Message ID | 20230608164437.5575-1-palmer@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Stop referencing __global_pointer$ under PIC | expand |
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
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
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
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
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.
* 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
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
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.
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.)
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.) > > -- > 宋方睿
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
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(+)