| Message ID | 20170820222602.1f51bd7c@windsurf |
|---|---|
| State | Not Applicable |
| Headers | show |
Thomas, Romain, Vicente, All, On 2017-08-20 22:26 +0200, Thomas Petazzoni spake thusly: > On Sun, 20 Aug 2017 16:41:54 +0200, Romain Naour wrote: > > I tried to use BR2_MIPS_NAN_2008 but it trigger a circular dependency in Kconfig. > I indeed tried: [--SNIP--] > And Kconfig yells with: > > toolchain/toolchain-buildroot/Config.in:23:error: recursive dependency detected! > toolchain/toolchain-buildroot/Config.in:23: choice <choice> contains symbol BR2_TOOLCHAIN_BUILDROOT_GLIBC > toolchain/toolchain-buildroot/Config.in:43: symbol BR2_TOOLCHAIN_BUILDROOT_GLIBC depends on BR2_MIPS_NAN_2008 > arch/Config.in.mips:168: symbol BR2_MIPS_NAN_2008 is selected by BR2_MIPS_ENABLE_NAN_2008 > arch/Config.in.mips:185: symbol BR2_MIPS_ENABLE_NAN_2008 is part of choice <choice> > arch/Config.in.mips:171: choice <choice> contains symbol <choice> > arch/Config.in.mips:171: choice <choice> contains symbol BR2_TOOLCHAIN_HAS_MNAN_OPTION > toolchain/toolchain-common.in:347: symbol BR2_TOOLCHAIN_HAS_MNAN_OPTION depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 > toolchain/toolchain-common.in:314: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_5 > toolchain/toolchain-common.in:318: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_5 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_6 > toolchain/toolchain-common.in:322: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_6 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_7 > toolchain/toolchain-common.in:326: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_7 is selected by BR2_GCC_VERSION_7_X > package/gcc/Config.in.host:68: symbol BR2_GCC_VERSION_7_X is part of choice <choice> > package/gcc/Config.in.host:3: choice <choice> contains symbol BR2_GCC_VERSION_4_9_X > package/gcc/Config.in.host:23: symbol BR2_GCC_VERSION_4_9_X depends on BR2_TOOLCHAIN_USES_MUSL > toolchain/Config.in:25: symbol BR2_TOOLCHAIN_USES_MUSL is selected by BR2_TOOLCHAIN_BUILDROOT_MUSL > toolchain/toolchain-buildroot/Config.in:75: symbol BR2_TOOLCHAIN_BUILDROOT_MUSL is part of choice <choice> [--SNIP--] > And I believe he is right to yell about this. I think the addition of > the BR2_TOOLCHAIN_HAS_<xyz>_OPTION dependencies in arch/Config.in.mips > is what causes the problem. Indeed, it's the first time we have a > dependency on an architecture option to a toolchain option. This is not > logical, because in Buildroot, you first select the architecture, and > then you select the toolchain details. For example, we disallow > selecting a given gcc version if it is not supported on a given > architecture. But here, those BR2_TOOLCHAIN_HAS_MFPXX_OPTION / > BR2_TOOLCHAIN_HAS_MNAN_OPTION are doing the opposite: they prevent you > from selecting a given architecture variant if the gcc version is not > sufficient. > > I believe we need to invert those dependencies, and instead have a > mechanism where selecting a too old gcc version is not possible when > -mnan/-mfp are used on MIPS. Agreed. > Yann is specifically working on a mechanism to allow architectures to > describe what minimum gcc version they need. Perhaps we should use that > to express this dependency as well. Adding Yann in Cc :) And indeed, it was pretty trivial to do so with my changes: https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=96c77a38f423fd1745ea5601b71d3ac63c7ea11e which allowed for: https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=81fb80dc8fc8b93754cc068552a1eae917f14842 Also, I think we should rename BR2_TOOLCHAIN_HAS_MNAN_OPTION to include the fact that it is mips, like: BR2_TOOLCHAIN_HAS_MIPS_MNAN_OPTION. Ditto BR2_TOOLCHAIN_HAS_MFPXX_OPTION, BR2_TOOLCHAIN_HAS_MIPS_MFPXX_OPTION. Regards, Yann E. MORIN.
Hello, On Mon, 21 Aug 2017 18:28:46 +0200, Yann E. MORIN wrote: > > Yann is specifically working on a mechanism to allow architectures to > > describe what minimum gcc version they need. Perhaps we should use that > > to express this dependency as well. Adding Yann in Cc :) > > And indeed, it was pretty trivial to do so with my changes: > https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=96c77a38f423fd1745ea5601b71d3ac63c7ea11e I am not sure this patch is correct. Indeed, in the choice you have two options: NaN 2008 and NaN legacy. With your patch, it's only when NaN 2008 is chosen that you indicate the architecture needs at least gcc 4.9. But in fact, even if NaN legacy is chosen, you need gcc 4.9, because -mnan=legacy also only works with gcc 4.9: the -mnan option didn't exist at all before gcc 4.9 (at least that's my understanding). However, I believe the current code is already bogus. Indeed, BR2_MIPS_CPU_MIPS32 selects BR2_MIPS_NAN_LEGACY, which means BR2_GCC_TARGET_NAN is set to legacy, so -mnan=legacy will be passed... which will break the build for gcc < 4.9. There is definitely something to double check here. > Also, I think we should rename BR2_TOOLCHAIN_HAS_MNAN_OPTION to include > the fact that it is mips, like: BR2_TOOLCHAIN_HAS_MIPS_MNAN_OPTION. Also what I thought when reviewing Vicente patches, but then I decided to not do it, because the gcc option is really named -mnan and not -mmips-nan or something like that. Thomas
Thomas, All, On 2017-08-21 23:10 +0200, Thomas Petazzoni spake thusly: > On Mon, 21 Aug 2017 18:28:46 +0200, Yann E. MORIN wrote: > > > > Yann is specifically working on a mechanism to allow architectures to > > > describe what minimum gcc version they need. Perhaps we should use that > > > to express this dependency as well. Adding Yann in Cc :) > > > > And indeed, it was pretty trivial to do so with my changes: > > https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=96c77a38f423fd1745ea5601b71d3ac63c7ea11e > > I am not sure this patch is correct. Indeed, in the choice you have two > options: NaN 2008 and NaN legacy. With your patch, it's only when NaN > 2008 is chosen that you indicate the architecture needs at least gcc > 4.9. > > But in fact, even if NaN legacy is chosen, you need gcc 4.9, because > -mnan=legacy also only works with gcc 4.9: the -mnan option didn't > exist at all before gcc 4.9 (at least that's my understanding). So, gcc before 4.9 do not have the -mnan option, so the symbol BR2_TOOLCHAIN_HAS_MNAN_OPTION is not set. And the code that adds -mnan is now conditioanl to that symbol. So it does work: 1- if the user selects NaN legacy, he is not restricted in the gcc version he may use; 1a- if gcc is older than 4.9, no -mnan option is passed, 1b- if gcc if 4.9 or later, -mnan=legacy is passed (although that is the default in gcc, we force it); 2- the user select NaN-2008, he is restricted in using a gcc 4.9 or later, and we always pass -mnan=2008. Isn't what we want? Of course, I may have left a bug somewhere... ;-) > However, I believe the current code is already bogus. Indeed, > BR2_MIPS_CPU_MIPS32 selects BR2_MIPS_NAN_LEGACY, which means > BR2_GCC_TARGET_NAN is set to legacy, so -mnan=legacy will be passed... > which will break the build for gcc < 4.9. Not really, because when gcc is older than 4.9, BR2_GCC_TARGET_NAN is not set, so this condition is not met (line ~210): package/gcc/gcc.mk:ifneq ($(call qstrip,$(BR2_GCC_TARGET_NAN)),) package/gcc/gcc.mk:HOST_GCC_COMMON_CONF_OPTS += --with-nan=$(BR2_GCC_TARGET_NAN) package/gcc/gcc.mk:endif So there is no -mnan option pased in this case. But with my patch, BR2_GCC_TARGET_NAN is always set now, so the condition above is now enclosed with: package/gcc/gcc.mk:ifeq ($(BR2_TOOLCHAIN_MIPS_HAS_MNAN_OPTION),y) [the code above] package/gcc/gcc.mk:endif > There is definitely something to double check here. > > > Also, I think we should rename BR2_TOOLCHAIN_HAS_MNAN_OPTION to include > > the fact that it is mips, like: BR2_TOOLCHAIN_HAS_MIPS_MNAN_OPTION. > > Also what I thought when reviewing Vicente patches, but then I decided > to not do it, because the gcc option is really named -mnan and not > -mmips-nan or something like that. Hence I renamed it BR2_TOOLCHAIN_MIPS_HAS_MNAN_OPTION because we already have symbols named as such: https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=b8950fddae941f427ce7b4cc64d6388573a93101 (We're not doing a review of a series I haven't yet posted, are we? ;-] ) Regards, Yann E. MORIN.
Yann, Thomas, All Le 22/08/2017 à 18:34, Yann E. MORIN a écrit : > Thomas, All, > > On 2017-08-21 23:10 +0200, Thomas Petazzoni spake thusly: >> On Mon, 21 Aug 2017 18:28:46 +0200, Yann E. MORIN wrote: >> >>>> Yann is specifically working on a mechanism to allow architectures to >>>> describe what minimum gcc version they need. Perhaps we should use that >>>> to express this dependency as well. Adding Yann in Cc :) >>> >>> And indeed, it was pretty trivial to do so with my changes: >>> https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=96c77a38f423fd1745ea5601b71d3ac63c7ea11e >> >> I am not sure this patch is correct. Indeed, in the choice you have two >> options: NaN 2008 and NaN legacy. With your patch, it's only when NaN >> 2008 is chosen that you indicate the architecture needs at least gcc >> 4.9. >> >> But in fact, even if NaN legacy is chosen, you need gcc 4.9, because >> -mnan=legacy also only works with gcc 4.9: the -mnan option didn't >> exist at all before gcc 4.9 (at least that's my understanding). > > So, gcc before 4.9 do not have the -mnan option, so the symbol > BR2_TOOLCHAIN_HAS_MNAN_OPTION is not set. And the code that adds -mnan > is now conditioanl to that symbol. > > So it does work: > > 1- if the user selects NaN legacy, he is not restricted in the gcc > version he may use; > > 1a- if gcc is older than 4.9, no -mnan option is passed, > 1b- if gcc if 4.9 or later, -mnan=legacy is passed (although that is > the default in gcc, we force it); > > 2- the user select NaN-2008, he is restricted in using a gcc 4.9 or > later, and we always pass -mnan=2008. > > Isn't what we want? > > Of course, I may have left a bug somewhere... ;-) > >> However, I believe the current code is already bogus. Indeed, >> BR2_MIPS_CPU_MIPS32 selects BR2_MIPS_NAN_LEGACY, which means >> BR2_GCC_TARGET_NAN is set to legacy, so -mnan=legacy will be passed... >> which will break the build for gcc < 4.9. > > Not really, because when gcc is older than 4.9, BR2_GCC_TARGET_NAN is > not set, so this condition is not met (line ~210): > > package/gcc/gcc.mk:ifneq ($(call qstrip,$(BR2_GCC_TARGET_NAN)),) > package/gcc/gcc.mk:HOST_GCC_COMMON_CONF_OPTS += --with-nan=$(BR2_GCC_TARGET_NAN) > package/gcc/gcc.mk:endif > > So there is no -mnan option pased in this case. > > But with my patch, BR2_GCC_TARGET_NAN is always set now, so the > condition above is now enclosed with: > > package/gcc/gcc.mk:ifeq ($(BR2_TOOLCHAIN_MIPS_HAS_MNAN_OPTION),y) > [the code above] > package/gcc/gcc.mk:endif > >> There is definitely something to double check here. >> >>> Also, I think we should rename BR2_TOOLCHAIN_HAS_MNAN_OPTION to include >>> the fact that it is mips, like: BR2_TOOLCHAIN_HAS_MIPS_MNAN_OPTION. >> >> Also what I thought when reviewing Vicente patches, but then I decided >> to not do it, because the gcc option is really named -mnan and not >> -mmips-nan or something like that. > > Hence I renamed it BR2_TOOLCHAIN_MIPS_HAS_MNAN_OPTION because we already > have symbols named as such: > > https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/aarch64-cpus-2&id=b8950fddae941f427ce7b4cc64d6388573a93101 > > (We're not doing a review of a series I haven't yet posted, are we? ;-] ) I'm glad to see you're already working on a fix. I'm still working on glibc 2.26 bump (hence this patch) but I'm facing to a build issue on x86_64 with mesa3d package. I'll take a look at your series as soon as it'll be on the list :) Best regards, Romain > > Regards, > Yann E. MORIN. >
diff --git a/toolchain/toolchain-buildroot/Config.in b/toolchain/toolchain-buildroot/Config.in index f47001f..9b25809 100644 --- a/toolchain/toolchain-buildroot/Config.in +++ b/toolchain/toolchain-buildroot/Config.in @@ -50,7 +50,8 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC BR2_microblaze || BR2_nios2 depends on BR2_USE_MMU depends on !BR2_STATIC_LIBS - depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2 + depends on (BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2 && !BR2_MIPS_NAN_2008) || \ + (BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 && BR2_MIPS_NAN_2008) depends on !BR2_powerpc_SPE select BR2_TOOLCHAIN_USES_GLIBC # our glibc.mk enables RPC support @@ -63,8 +64,14 @@ config BR2_TOOLCHAIN_BUILDROOT_GLIBC comment "glibc needs a toolchain w/ dynamic library, kernel headers >= 3.2" depends on BR2_USE_MMU + depends on !BR2_MIPS_NAN_2008 depends on BR2_STATIC_LIBS || !BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_2 +comment "glibc needs a toolchain w/ dynamic library, kernel headers >= 4.5" + depends on BR2_USE_MMU + depends on BR2_MIPS_NAN_2008 + depends on BR2_STATIC_LIBS || !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_5 + config BR2_TOOLCHAIN_BUILDROOT_MUSL bool "musl" depends on BR2_aarch64 || BR2_arm || BR2_armeb || BR2_i386 || \
Hello, +Vicente, since the discussion is MIPS related. On Sun, 20 Aug 2017 16:41:54 +0200, Romain Naour wrote: > I tried to use BR2_MIPS_NAN_2008 but it trigger a circular dependency in Kconfig. I indeed tried: And Kconfig yells with: toolchain/toolchain-buildroot/Config.in:23:error: recursive dependency detected! toolchain/toolchain-buildroot/Config.in:23: choice <choice> contains symbol BR2_TOOLCHAIN_BUILDROOT_GLIBC toolchain/toolchain-buildroot/Config.in:43: symbol BR2_TOOLCHAIN_BUILDROOT_GLIBC depends on BR2_MIPS_NAN_2008 arch/Config.in.mips:168: symbol BR2_MIPS_NAN_2008 is selected by BR2_MIPS_ENABLE_NAN_2008 arch/Config.in.mips:185: symbol BR2_MIPS_ENABLE_NAN_2008 is part of choice <choice> arch/Config.in.mips:171: choice <choice> contains symbol <choice> arch/Config.in.mips:171: choice <choice> contains symbol BR2_TOOLCHAIN_HAS_MNAN_OPTION toolchain/toolchain-common.in:347: symbol BR2_TOOLCHAIN_HAS_MNAN_OPTION depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 toolchain/toolchain-common.in:314: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_5 toolchain/toolchain-common.in:318: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_5 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_6 toolchain/toolchain-common.in:322: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_6 is selected by BR2_TOOLCHAIN_GCC_AT_LEAST_7 toolchain/toolchain-common.in:326: symbol BR2_TOOLCHAIN_GCC_AT_LEAST_7 is selected by BR2_GCC_VERSION_7_X package/gcc/Config.in.host:68: symbol BR2_GCC_VERSION_7_X is part of choice <choice> package/gcc/Config.in.host:3: choice <choice> contains symbol BR2_GCC_VERSION_4_9_X package/gcc/Config.in.host:23: symbol BR2_GCC_VERSION_4_9_X depends on BR2_TOOLCHAIN_USES_MUSL toolchain/Config.in:25: symbol BR2_TOOLCHAIN_USES_MUSL is selected by BR2_TOOLCHAIN_BUILDROOT_MUSL toolchain/toolchain-buildroot/Config.in:75: symbol BR2_TOOLCHAIN_BUILDROOT_MUSL is part of choice <choice> And I believe he is right to yell about this. I think the addition of the BR2_TOOLCHAIN_HAS_<xyz>_OPTION dependencies in arch/Config.in.mips is what causes the problem. Indeed, it's the first time we have a dependency on an architecture option to a toolchain option. This is not logical, because in Buildroot, you first select the architecture, and then you select the toolchain details. For example, we disallow selecting a given gcc version if it is not supported on a given architecture. But here, those BR2_TOOLCHAIN_HAS_MFPXX_OPTION / BR2_TOOLCHAIN_HAS_MNAN_OPTION are doing the opposite: they prevent you from selecting a given architecture variant if the gcc version is not sufficient. I believe we need to invert those dependencies, and instead have a mechanism where selecting a too old gcc version is not possible when -mnan/-mfp are used on MIPS. Yann is specifically working on a mechanism to allow architectures to describe what minimum gcc version they need. Perhaps we should use that to express this dependency as well. Adding Yann in Cc :) Thomas