| Message ID | 20210311214633.2727-1-cotequeiroz@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gmp: compile with -DPIC to use correct asm code | expand |
On 11/03/2021 23:46, Eneas U de Queiroz wrote: > The library is always compiled with $(FPIC) (-fPIC or -fpic), even for > the static library. > > There are some assembly sources that decide whether or not to enable > PIC code by checking if PIC is defined. It counts on libtool to define > it, but libtool does it only when producing code for the dynamic > library, while we need it for both. > > Ensure it is defined by adding it to CFLAGS next to $(FPIC). > > It avoids linking errors with strongswan on x86_64: > > ld: libgmp.a(bdiv_q_1.o): relocation R_X86_64_PC32 against symbol > `__gmp_binvert_limb_table' can not be used when making a shared object; > recompile with -fPIC > > Cc: Stijn Tintel <stijn@linux-ipv6.be> > Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> > --- > > There's an error on one architecture, and all others work fine without > this, so I'm uneasy changing this and then breaking stuff that was > working fine otherwise. However, it feels wrong to me to generate PIC > code from C files, but not use it in asm sources, which is essentially > what I am changing here. > > I've looked at asm sources for different chitectures, and there are > checks for PIC in: arm64, arm, x86_64, x86, and ppc asm sources, but the > error only appears on x86_64. > > For most CPUs, ifdef(`PIC'), is just used to do different definitions of > LEA (Load Effective Address). However, both x86 and x86_64 have many > other checks. > > I've looked at bdiv_q_1.asm for different CPUs, and they all do some > form of LEA(binvert_limb_table), except for x86, where it will do it > only when PIC is defined. That may explain why x86_64 is affected, and > x86 is not. > > I have not investigated further details. > > Alternatively, we can define it only for x86_64, which is where we know > there's a build failure with the linker asking to recompile with -fPIC. > I'm sorry, but I lack the knowledge to review this. Stijn
On 2021-03-12 11:34, Stijn Tintel wrote: > On 11/03/2021 23:46, Eneas U de Queiroz wrote: >> The library is always compiled with $(FPIC) (-fPIC or -fpic), even for >> the static library. >> >> There are some assembly sources that decide whether or not to enable >> PIC code by checking if PIC is defined. It counts on libtool to define >> it, but libtool does it only when producing code for the dynamic >> library, while we need it for both. >> >> Ensure it is defined by adding it to CFLAGS next to $(FPIC). >> >> It avoids linking errors with strongswan on x86_64: >> >> ld: libgmp.a(bdiv_q_1.o): relocation R_X86_64_PC32 against symbol >> `__gmp_binvert_limb_table' can not be used when making a shared object; >> recompile with -fPIC >> >> Cc: Stijn Tintel <stijn@linux-ipv6.be> >> Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> >> --- >> >> There's an error on one architecture, and all others work fine without >> this, so I'm uneasy changing this and then breaking stuff that was >> working fine otherwise. However, it feels wrong to me to generate PIC >> code from C files, but not use it in asm sources, which is essentially >> what I am changing here. >> >> I've looked at asm sources for different chitectures, and there are >> checks for PIC in: arm64, arm, x86_64, x86, and ppc asm sources, but the >> error only appears on x86_64. >> >> For most CPUs, ifdef(`PIC'), is just used to do different definitions of >> LEA (Load Effective Address). However, both x86 and x86_64 have many >> other checks. >> >> I've looked at bdiv_q_1.asm for different CPUs, and they all do some >> form of LEA(binvert_limb_table), except for x86, where it will do it >> only when PIC is defined. That may explain why x86_64 is affected, and >> x86 is not. >> >> I have not investigated further details. >> >> Alternatively, we can define it only for x86_64, which is where we know >> there's a build failure with the linker asking to recompile with -fPIC. >> > I'm sorry, but I lack the knowledge to review this. I think the patch makes sense and -DPIC should be used along with -fPIC. I don't see any reason to make it x86_64 specific. - Felix
> On Mar 12, 2021, at 6:25 AM, Felix Fietkau <nbd@nbd.name> wrote: > > > On 2021-03-12 11:34, Stijn Tintel wrote: >> On 11/03/2021 23:46, Eneas U de Queiroz wrote: >>> The library is always compiled with $(FPIC) (-fPIC or -fpic), even for >>> the static library. >>> >>> There are some assembly sources that decide whether or not to enable >>> PIC code by checking if PIC is defined. It counts on libtool to define >>> it, but libtool does it only when producing code for the dynamic >>> library, while we need it for both. >>> >>> Ensure it is defined by adding it to CFLAGS next to $(FPIC). >>> >>> It avoids linking errors with strongswan on x86_64: >>> >>> ld: libgmp.a(bdiv_q_1.o): relocation R_X86_64_PC32 against symbol >>> `__gmp_binvert_limb_table' can not be used when making a shared object; >>> recompile with -fPIC >>> >>> Cc: Stijn Tintel <stijn@linux-ipv6.be> >>> Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> >>> --- >>> >>> There's an error on one architecture, and all others work fine without >>> this, so I'm uneasy changing this and then breaking stuff that was >>> working fine otherwise. However, it feels wrong to me to generate PIC >>> code from C files, but not use it in asm sources, which is essentially >>> what I am changing here. >>> >>> I've looked at asm sources for different chitectures, and there are >>> checks for PIC in: arm64, arm, x86_64, x86, and ppc asm sources, but the >>> error only appears on x86_64. >>> >>> For most CPUs, ifdef(`PIC'), is just used to do different definitions of >>> LEA (Load Effective Address). However, both x86 and x86_64 have many >>> other checks. >>> >>> I've looked at bdiv_q_1.asm for different CPUs, and they all do some >>> form of LEA(binvert_limb_table), except for x86, where it will do it >>> only when PIC is defined. That may explain why x86_64 is affected, and >>> x86 is not. >>> >>> I have not investigated further details. >>> >>> Alternatively, we can define it only for x86_64, which is where we know >>> there's a build failure with the linker asking to recompile with -fPIC. >>> >> I'm sorry, but I lack the knowledge to review this. > > I think the patch makes sense and -DPIC should be used along with -fPIC. > I don't see any reason to make it x86_64 specific. > > - Felix Maybe I'm missing something, but why not just fix rules.mk: ifneq (,$(findstring $(ARCH) , aarch64 aarch64_be powerpc )) FPIC:=-fPIC else FPIC:=-fpic endif HOST_FPIC:=-fPIC To have the FPIC and HOST_FPIC definitions include -DPIC?
On Fri, Mar 19, 2021 at 5:08 PM Philip Prindeville <philipp_subx@redfish-solutions.com> wrote: > > > Maybe I'm missing something, but why not just fix rules.mk: > > > ifneq (,$(findstring $(ARCH) , aarch64 aarch64_be powerpc )) > FPIC:=-fPIC > else > FPIC:=-fpic > endif > > HOST_FPIC:=-fPIC > > > To have the FPIC and HOST_FPIC definitions include -DPIC? I think it would be the proper way to handle this. I was initially fearful of changing too much and breaking things, but I think it should be expected behaviour. What else would you use a 'PIC' definition for? I will resend a patch changing rules.mk instead.
> On Mar 19, 2021, at 4:06 PM, Eneas U de Queiroz <cotequeiroz@gmail.com> wrote: > > On Fri, Mar 19, 2021 at 5:08 PM Philip Prindeville > <philipp_subx@redfish-solutions.com> wrote: >> >> >> Maybe I'm missing something, but why not just fix rules.mk: >> >> >> ifneq (,$(findstring $(ARCH) , aarch64 aarch64_be powerpc )) >> FPIC:=-fPIC >> else >> FPIC:=-fpic >> endif >> >> HOST_FPIC:=-fPIC >> >> >> To have the FPIC and HOST_FPIC definitions include -DPIC? > > I think it would be the proper way to handle this. I was initially > fearful of changing too much and breaking things, but I think it > should be expected behaviour. What else would you use a 'PIC' > definition for? I will resend a patch changing rules.mk instead. Just saw this... Turns out that this would break a couple of packages (both in openwrt and in packages), so I've included those as well. -Philip
> On Mar 19, 2021, at 4:59 PM, Philip Prindeville <philipp_subx@redfish-solutions.com> wrote: > > > >> On Mar 19, 2021, at 4:06 PM, Eneas U de Queiroz <cotequeiroz@gmail.com> wrote: >> >> On Fri, Mar 19, 2021 at 5:08 PM Philip Prindeville >> <philipp_subx@redfish-solutions.com> wrote: >>> >>> >>> Maybe I'm missing something, but why not just fix rules.mk: >>> >>> >>> ifneq (,$(findstring $(ARCH) , aarch64 aarch64_be powerpc )) >>> FPIC:=-fPIC >>> else >>> FPIC:=-fpic >>> endif >>> >>> HOST_FPIC:=-fPIC >>> >>> >>> To have the FPIC and HOST_FPIC definitions include -DPIC? >> >> I think it would be the proper way to handle this. I was initially >> fearful of changing too much and breaking things, but I think it >> should be expected behaviour. What else would you use a 'PIC' >> definition for? I will resend a patch changing rules.mk instead. > > > Just saw this... Turns out that this would break a couple of packages (both in openwrt and in packages), so I've included those as well. > > -Philip > Hi all, I think that Eneas and I have converged on a fix, it should help with some of the CircleCI issues we've been having with x86_64 (which oddly isn't our most leveraged architecture for CI/CD testing, even though it's the easiest to cloud deploy... but that's a separate discussion). We've both built and tested with this patch and no ill effects. There's actually a surprising number of places where the symbol "PIC" is tested, not just in asm files (which was the original libgmp problem), but also in C files supporting dlopen(), stack unwinding, etc. If you go into build_dir/target-*/ and run: % find . \( -name "*.[hc]" -o -name "*.[hc]pp" -o -name "*.asm" \) -print | xargs egrep -r -n '\<if.*\<PIC\>' You'll get about 200+ hits (modulo some occurrences of "PIC" in comments), very close to 50% of those being in gmp-6.2.1 itself (not surprising because it has a lot of asm code). Other hits are also where you'd expect them: elfutils, binutils, gdb, etc. i.e. things that need to be relocation-aware or else projects with plugin support in the form of .so's (such as cyrus-sasl). The fix itself is trivial: it's 3 lines of changes. Can we please get core reviewers to consider and merge this? https://github.com/openwrt/openwrt/pull/4006 Thanks! -Philip
diff --git a/package/libs/gmp/Makefile b/package/libs/gmp/Makefile index eb7d808139..d59e8fe947 100644 --- a/package/libs/gmp/Makefile +++ b/package/libs/gmp/Makefile @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=gmp PKG_VERSION:=6.2.1 -PKG_RELEASE:=1 +PKG_RELEASE:=2 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION)$(PKG_REVISION).tar.xz PKG_SOURCE_URL:=@GNU/gmp/ @@ -38,7 +38,7 @@ define Package/libgmp/description signed integers, rational numbers, and floating point numbers. endef -TARGET_CFLAGS += $(FPIC) +TARGET_CFLAGS += -DPIC $(FPIC) CONFIGURE_VARS += CC="$(TARGET_CROSS)gcc" CONFIGURE_ARGS += \ --enable-shared \
The library is always compiled with $(FPIC) (-fPIC or -fpic), even for the static library. There are some assembly sources that decide whether or not to enable PIC code by checking if PIC is defined. It counts on libtool to define it, but libtool does it only when producing code for the dynamic library, while we need it for both. Ensure it is defined by adding it to CFLAGS next to $(FPIC). It avoids linking errors with strongswan on x86_64: ld: libgmp.a(bdiv_q_1.o): relocation R_X86_64_PC32 against symbol `__gmp_binvert_limb_table' can not be used when making a shared object; recompile with -fPIC Cc: Stijn Tintel <stijn@linux-ipv6.be> Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> --- There's an error on one architecture, and all others work fine without this, so I'm uneasy changing this and then breaking stuff that was working fine otherwise. However, it feels wrong to me to generate PIC code from C files, but not use it in asm sources, which is essentially what I am changing here. I've looked at asm sources for different chitectures, and there are checks for PIC in: arm64, arm, x86_64, x86, and ppc asm sources, but the error only appears on x86_64. For most CPUs, ifdef(`PIC'), is just used to do different definitions of LEA (Load Effective Address). However, both x86 and x86_64 have many other checks. I've looked at bdiv_q_1.asm for different CPUs, and they all do some form of LEA(binvert_limb_table), except for x86, where it will do it only when PIC is defined. That may explain why x86_64 is affected, and x86 is not. I have not investigated further details. Alternatively, we can define it only for x86_64, which is where we know there's a build failure with the linker asking to recompile with -fPIC. package/libs/gmp/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)