| Message ID | 1442500678-4457-1-git-send-email-brendanheading@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Brendan, On Thu, 17 Sep 2015 15:37:58 +0100, Brendan Heading wrote: > GCC's configure stage assumes that if the glibc version, as denoted by > __GLIBC__ and __GLIBC_MINOR__, is greater or equal to 2.4 then stack > protection must be available in the C library. This results in the compiler > not attempting to link SSP helpers during the final link. > > The problem is seen with uclibc-ng 1.0.6 (and likely greater) where they > updated the __GLIBC_MINOR__ value to 10. It will be seen in any libc > that permits stack protection to be disabled while exporting a glibc > version >= 2.4. Cc'ing Waldemar here. It's an interesting consequence of bumping the GLIBC_MINOR version exposed by uClibc, which happened recently to solve an eventfd_read() problem in Boost on ARC, investigated by Alexey (also in Cc). > > This patch overrides GCC to expect/not expect SSP support in libc based on > the toolchain's capability as understood by buildroot. > > Signed-off-by: Brendan Heading <brendanheading@gmail.com> > --- > Patch V1 : > This fix definitely solves the problem, however I doubt it's acceptable to > export environment variables in this way. > > I had initially tried adding it to HOST_GCC_COMMON_CONF_ENV, and I > confirmed that this is passed into GCC's top level configure, however it > does not seem to propagate to the gcc/configure script. Another way would > be to patch GCC, however this would involve maintaining patches for all > the supported GCC versions. > > Improvement suggestions welcome! > --- > package/gcc/gcc.mk | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/package/gcc/gcc.mk b/package/gcc/gcc.mk > index 501fcea..43835af 100644 > --- a/package/gcc/gcc.mk > +++ b/package/gcc/gcc.mk > @@ -123,6 +123,14 @@ endif > HOST_GCC_COMMON_CONF_ENV += CFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CFLAGS)" > HOST_GCC_COMMON_CONF_ENV += CXXFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CXXFLAGS)" > > +# Work around issue with detecting SSP support in the C library > + > +ifeq ($(BR2_TOOLCHAIN_HAS_SSP),y) > +export gcc_cv_libc_provides_ssp=yes > +else > +export gcc_cv_libc_provides_ssp=no > +endif Exporting that globally is really horrible. I think the appropriate fix is probably to teach gcc about uClibc, like we're doing for musl already (http://git.buildroot.net/buildroot/tree/package/gcc/4.9.3/900-musl-support.patch#n452). So we could do a check here like: *uclibc*) test if __UCLIBC_HAS_SSP__ is defined or not. If yes -> we have SSP, if not -> we don't have SSP. And contribute that to upstream gcc. What do you think? Thomas
>> The problem is seen with uclibc-ng 1.0.6 (and likely greater) where they >> updated the __GLIBC_MINOR__ value to 10. It will be seen in any libc >> that permits stack protection to be disabled while exporting a glibc >> version >= 2.4. > > Cc'ing Waldemar here. It's an interesting consequence of bumping the > GLIBC_MINOR version exposed by uClibc, which happened recently to solve > an eventfd_read() problem in Boost on ARC, investigated by Alexey (also > in Cc). Yup, that's exactly what has exposed the problem. And the "problem" is really a deficiency in the way GCC detects SSP support in the libc. > Exporting that globally is really horrible. Yeah I knew that exporting it globally was not going to fly - I just submitted it as a starting point for discussion how to solve this. > I think the appropriate fix > is probably to teach gcc about uClibc, like we're doing for musl already > (http://git.buildroot.net/buildroot/tree/package/gcc/4.9.3/900-musl-support.patch#n452). > So we could do a check here like: > > *uclibc*) > test if __UCLIBC_HAS_SSP__ is defined or not. If yes -> we have > SSP, if not -> we don't have SSP. > > And contribute that to upstream gcc. > > What do you think? It is up to you. My only quibble would be that layering another patch in this area (where there are already patches adding stuff as you noted) might get a bit fiddly. Are we okay with patches being order-dependent ? This does mean that they're not so easy to submit upstream .. Aside from that .. GCC actually already has a block which checks UCLIBC_HAS_SSP. The problem is that it reaches it only if the glibc version check returns that the version is 2.3 or lower. The fix might simply be to reorder the check. I'm curious why GCC have done this rather than doing a check for the __stack_chk symbols. Would be interested in hearing from Alexey and Waldemar. Brendan
Hi, Brendan Heading wrote, > Aside from that .. GCC actually already has a block which checks > UCLIBC_HAS_SSP. The problem is that it reaches it only if the glibc > version check returns that the version is 2.3 or lower. The fix might > simply be to reorder the check. > > I'm curious why GCC have done this rather than doing a check for the > __stack_chk symbols. Would be interested in hearing from Alexey and > Waldemar. I vote for the GCC patch, if there is already a __UCLIBC_HAS_SSP__ check. best regards Waldemar
Brendan, On Thu, 17 Sep 2015 18:29:12 +0100, Brendan Heading wrote: > > Cc'ing Waldemar here. It's an interesting consequence of bumping the > > GLIBC_MINOR version exposed by uClibc, which happened recently to solve > > an eventfd_read() problem in Boost on ARC, investigated by Alexey (also > > in Cc). > > Yup, that's exactly what has exposed the problem. And the "problem" is > really a deficiency in the way GCC detects SSP support in the libc. Correct. > > Exporting that globally is really horrible. > > Yeah I knew that exporting it globally was not going to fly - I just > submitted it as a starting point for discussion how to solve this. Sure, no problem with that. It's already much appreciated that you did all this investigation. > > I think the appropriate fix > > is probably to teach gcc about uClibc, like we're doing for musl already > > (http://git.buildroot.net/buildroot/tree/package/gcc/4.9.3/900-musl-support.patch#n452). > > So we could do a check here like: > > > > *uclibc*) > > test if __UCLIBC_HAS_SSP__ is defined or not. If yes -> we have > > SSP, if not -> we don't have SSP. > > > > And contribute that to upstream gcc. > > > > What do you think? > > It is up to you. My only quibble would be that layering another patch > in this area (where there are already patches adding stuff as you > noted) might get a bit fiddly. Are we okay with patches being > order-dependent ? This does mean that they're not so easy to submit > upstream .. We are fine with patches being order-dependent. That's fine we have a sequence number for all patches in the first place. So far, the effort to push upstream our gcc patches has been very limited. It would be good to push some of them upstream, but in the mean time, our stack of gcc patches is not that big, and is not causing too many problems when bumping gcc. > Aside from that .. GCC actually already has a block which checks > UCLIBC_HAS_SSP. The problem is that it reaches it only if the glibc > version check returns that the version is 2.3 or lower. The fix might > simply be to reorder the check. If it's that simple, then it should be done :) In any case, version based tests are often not a good idea, so when uClibc is used, gcc should really rely on UCLIBC_HAS_SSP. Thomas
>> Yeah I knew that exporting it globally was not going to fly - I just >> submitted it as a starting point for discussion how to solve this. > > Sure, no problem with that. It's already much appreciated that you did > all this investigation. Thank you for that! > We are fine with patches being order-dependent. That's fine we have a > sequence number for all patches in the first place. > > So far, the effort to push upstream our gcc patches has been very > limited. It would be good to push some of them upstream, but in the > mean time, our stack of gcc patches is not that big, and is not causing > too many problems when bumping gcc. Yeah I also get the sense that getting patches upstream in GCC might be difficult, and when I was googling this I saw other examples of people running into problems similar to this. I would guess it's especially unlikely we would get patches into the versions that are in maintenance mode. We might have more luck with next (ie GCC 6.0). >> Aside from that .. GCC actually already has a block which checks >> UCLIBC_HAS_SSP. The problem is that it reaches it only if the glibc >> version check returns that the version is 2.3 or lower. The fix might >> simply be to reorder the check. > > If it's that simple, then it should be done :) In any case, version > based tests are often not a good idea, so when uClibc is used, gcc > should really rely on UCLIBC_HAS_SSP. Okay, I am now looking at putting together an interim patch set. I don't think there is any point in trying to submit them into maintenance releases of the existing versions but it's worth taking a shot at getting them into -next. regards Brendan
Hi Brendan, On Thu, 2015-09-17 at 18:29 +0100, Brendan Heading wrote: The problem is seen with uclibc-ng 1.0.6 (and likely greater) where they > > > updated the __GLIBC_MINOR__ value to 10. It will be seen in any libc > > > that permits stack protection to be disabled while exporting a glibc > > > version >= 2.4. > > > > Cc'ing Waldemar here. It's an interesting consequence of bumping the > > GLIBC_MINOR version exposed by uClibc, which happened recently to solve > > an eventfd_read() problem in Boost on ARC, investigated by Alexey (also > > in Cc). > > Yup, that's exactly what has exposed the problem. And the "problem" is > really a deficiency in the way GCC detects SSP support in the libc. Well I expected new issues to show up after that change but I'm surprised it's the first one reported - so IMHO we're dealing quite well. As for the nature of that new problem I'd say it's also expected. Because I see people use glibc version to determine which feature or set of features could be used. And IMHO that's not very correct especially compared to checking each particular feature via compilation. And frankly I was not very happy with that blind bump of GLIBC_MINOR from one magical version (I was not able to find any justification why it was the value it was) to another not justified also... just in attempt to pretend we're now covering more things in uClibc. But it was low hanging fruit and so we decided to go that way instead of fixing affected projects... which in its turn could be not that easy as patch submission in either uClibc or Buildroot :) > __stack_chk symbols. Would be interested in hearing from Alexey and > Waldemar I think I'll vote for GCC patch as well. So there will be a check for __UCLIBC_HAS_SSP__ regardless that GLIBC_MINOR value. -Alexey
Brendan, On Thu, 17 Sep 2015 22:22:52 +0100, Brendan Heading wrote: > > We are fine with patches being order-dependent. That's fine we have a > > sequence number for all patches in the first place. > > > > So far, the effort to push upstream our gcc patches has been very > > limited. It would be good to push some of them upstream, but in the > > mean time, our stack of gcc patches is not that big, and is not causing > > too many problems when bumping gcc. > > Yeah I also get the sense that getting patches upstream in GCC might > be difficult, and when I was googling this I saw other examples of > people running into problems similar to this. I would guess it's > especially unlikely we would get patches into the versions that are in > maintenance mode. We might have more luck with next (ie GCC 6.0). It is indeed not necessarily easy to get patches upstream, but when it is the proper fix, it is nonetheless what we should do. > >> Aside from that .. GCC actually already has a block which checks > >> UCLIBC_HAS_SSP. The problem is that it reaches it only if the glibc > >> version check returns that the version is 2.3 or lower. The fix might > >> simply be to reorder the check. > > > > If it's that simple, then it should be done :) In any case, version > > based tests are often not a good idea, so when uClibc is used, gcc > > should really rely on UCLIBC_HAS_SSP. > > Okay, I am now looking at putting together an interim patch set. I > don't think there is any point in trying to submit them into > maintenance releases of the existing versions but it's worth taking a > shot at getting them into -next. Great, thanks! In fact, maybe uClibc-ng should have some sort of side project to collect the gcc patches needed to build a uClibc based toolchain, a bit like musl is doing at https://bitbucket.org/GregorR/musl-gcc-patches/src. Best regards, Thomas
diff --git a/package/gcc/gcc.mk b/package/gcc/gcc.mk index 501fcea..43835af 100644 --- a/package/gcc/gcc.mk +++ b/package/gcc/gcc.mk @@ -123,6 +123,14 @@ endif HOST_GCC_COMMON_CONF_ENV += CFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CFLAGS)" HOST_GCC_COMMON_CONF_ENV += CXXFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CXXFLAGS)" +# Work around issue with detecting SSP support in the C library + +ifeq ($(BR2_TOOLCHAIN_HAS_SSP),y) +export gcc_cv_libc_provides_ssp=yes +else +export gcc_cv_libc_provides_ssp=no +endif + # libitm needs sparc V9+ ifeq ($(BR2_sparc_v8)$(BR2_sparc_leon3),y) HOST_GCC_COMMON_CONF_OPTS += --disable-libitm
Fixes: http://autobuild.buildroot.net/results/123/123a5b3f72ba8c1a4aa1cea5b7b846a04fd4e923/ http://autobuild.buildroot.net/results/38c/38cfa4e7249a8770b06dbd392acba79303d3f9bc/ .. and others. GCC's configure stage assumes that if the glibc version, as denoted by __GLIBC__ and __GLIBC_MINOR__, is greater or equal to 2.4 then stack protection must be available in the C library. This results in the compiler not attempting to link SSP helpers during the final link. The problem is seen with uclibc-ng 1.0.6 (and likely greater) where they updated the __GLIBC_MINOR__ value to 10. It will be seen in any libc that permits stack protection to be disabled while exporting a glibc version >= 2.4. This patch overrides GCC to expect/not expect SSP support in libc based on the toolchain's capability as understood by buildroot. Signed-off-by: Brendan Heading <brendanheading@gmail.com> --- Patch V1 : This fix definitely solves the problem, however I doubt it's acceptable to export environment variables in this way. I had initially tried adding it to HOST_GCC_COMMON_CONF_ENV, and I confirmed that this is passed into GCC's top level configure, however it does not seem to propagate to the gcc/configure script. Another way would be to patch GCC, however this would involve maintaining patches for all the supported GCC versions. Improvement suggestions welcome! --- package/gcc/gcc.mk | 8 ++++++++ 1 file changed, 8 insertions(+)