| Message ID | 20180205211015.26819-2-thomas.petazzoni@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add gdb 8.1 support, switch to 8.0 as default, drop old versions | expand |
Thomas, All, On 2018-02-05 22:10 +0100, Thomas Petazzoni spake thusly: > As we are about to switch to 8.0 as the default gdb dependency, we > need to adjust how the gdb dependencies are handled. Indeed, from 8.0 > onwards, gdb needs a C++11 capable compiler, i.e at least gcc 4.8. > > Until now, Config.in.host was making sure that gdb 8.0 was not > selectable if the cross-compilation toolchain did not have C++ support > with gcc >= 4.8. This worked fine because the default version of gdb, > used as the target gdb version when no host gdb is built, was 7.11, > and did not require C++11. > > With the switch to 8.0 as the default version, when target gdb is > enabled but not host gdb, 8.0 is used, which means we need a C++11 > capable compiler. The dependencies in Config.in.host are no longer > sufficient. > > So instead, we remove the target-related dependencies from > Config.in.host and move them properly to Config.in. The overall logic > is the following: > > - In Config.in.host, BR2_PACKAGE_HOST_GDB_ARCH_SUPPORTS ensures that > we have at least host gcc 4.8 if we're on ARC, because the ARC gdb > needs C++11. We remove the target toolchain related dependencies > from here. > > - In Config.in.host, the version selection ensures that 8.0 cannot be > selected if the host toolchain does not have at least gcc 4.8. We > remove the target toolchain related dependencies from here. > > - In Config.in, we handle gdb more like a regular target package, > with a Config.in comment when its dependencies are not met. A > hidden boolean BR2_PACKAGE_GDB_NEEDS_CXX11 indicates when the > currently selected version requires C++11. Based on that, we show > the Config.in comment, and add the proper dependencies to > BR2_PACKAGE_GDB. It is worth mentioning that > BR2_PACKAGE_GDB_NEEDS_CXX11 is intentionally created to be !7.10 && > !7.11 && !7.12 instead of 8.0, because we will gradually add more > C++11-requiring versions, and remove non-C++11-requiring versions. Nice refactoring. :-) Yet, a comment below... > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > package/gdb/Config.in | 15 +++++++++++++++ > package/gdb/Config.in.host | 3 --- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/package/gdb/Config.in b/package/gdb/Config.in > index af020f40c5..b7f0565b92 100644 > --- a/package/gdb/Config.in > +++ b/package/gdb/Config.in > @@ -11,15 +11,30 @@ comment "gdb/gdbserver needs a toolchain w/ threads, threads debug" > depends on BR2_PACKAGE_GDB_ARCH_SUPPORTS > depends on !BR2_TOOLCHAIN_HAS_THREADS || !BR2_TOOLCHAIN_HAS_THREADS_DEBUG > > +config BR2_PACKAGE_GDB_NEEDS_CXX11 > + bool > + default y > + depends on !BR2_GDB_VERSION_7_10 > + depends on !BR2_GDB_VERSION_7_11 > + depends on !BR2_GDB_VERSION_7_12 I would have expected that the various versions would select this, i.e. something like: config BR2_PACKAGE_GDB_NEEDS_CXX11 bool config BR2_GDB_VERSION_7_10 bool "7.10" config BR2_GDB_VERSION_8_0 bool "8.0" select BR2_PACKAGE_GDB_NEEDS_CXX11 Otherwise, I'm OK with the patch. Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Regards, Yann E. MORIN. > +comment "gdb/gdbserver >= 8.x needs a toolchain w/ C++, gcc >= 4.8" > + depends on BR2_PACKAGE_GDB_NEEDS_CXX11 > + depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > + > config BR2_PACKAGE_GDB > bool "gdb" > depends on BR2_TOOLCHAIN_HAS_THREADS && BR2_TOOLCHAIN_HAS_THREADS_DEBUG > depends on BR2_PACKAGE_GDB_ARCH_SUPPORTS > + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_PACKAGE_GDB_NEEDS_CXX11 > + depends on BR2_INSTALL_LIBSTDCPP || !BR2_PACKAGE_GDB_NEEDS_CXX11 > # When the external toolchain gdbserver is copied to the > # target, we don't allow building a separate gdbserver. The > # one from the external toolchain should be used. > select BR2_PACKAGE_GDB_SERVER if \ > (!BR2_PACKAGE_GDB_DEBUGGER && !BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY) > + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_PACKAGE_GDB_NEEDS_CXX11 > + depends on BR2_INSTALL_LIBSTDCPP || !BR2_PACKAGE_GDB_NEEDS_CXX11 > help > GDB, the GNU Project debugger, allows you to see what is > going on `inside' another program while it executes -- or > diff --git a/package/gdb/Config.in.host b/package/gdb/Config.in.host > index 99e1cae5ba..8676c6f01b 100644 > --- a/package/gdb/Config.in.host > +++ b/package/gdb/Config.in.host > @@ -3,7 +3,6 @@ config BR2_PACKAGE_HOST_GDB_ARCH_SUPPORTS > default y > # The ARC version needs C++11, thus gcc >= 4.8, like gdb-8.0.x > depends on BR2_HOST_GCC_AT_LEAST_4_8 || !BR2_arc > - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_arc > depends on !((BR2_arm || BR2_armeb) && BR2_BINFMT_FLAT) > depends on !BR2_microblaze > depends on !BR2_nios2 > @@ -63,9 +62,7 @@ config BR2_GDB_VERSION_7_12 > config BR2_GDB_VERSION_8_0 > bool "gdb 8.0.x" > # Needs a C++11 compiler > - depends on BR2_INSTALL_LIBSTDCPP > depends on BR2_HOST_GCC_AT_LEAST_4_8 > - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > > endchoice > > -- > 2.14.3 >
Hello, On Wed, 7 Feb 2018 10:58:28 +0100, Yann E. MORIN wrote: > > - In Config.in, we handle gdb more like a regular target package, > > with a Config.in comment when its dependencies are not met. A > > hidden boolean BR2_PACKAGE_GDB_NEEDS_CXX11 indicates when the > > currently selected version requires C++11. Based on that, we show > > the Config.in comment, and add the proper dependencies to > > BR2_PACKAGE_GDB. It is worth mentioning that > > BR2_PACKAGE_GDB_NEEDS_CXX11 is intentionally created to be !7.10 && > > !7.11 && !7.12 instead of 8.0, because we will gradually add more > > C++11-requiring versions, and remove non-C++11-requiring versions. > > Nice refactoring. :-) Thanks! > > +config BR2_PACKAGE_GDB_NEEDS_CXX11 > > + bool > > + default y > > + depends on !BR2_GDB_VERSION_7_10 > > + depends on !BR2_GDB_VERSION_7_11 > > + depends on !BR2_GDB_VERSION_7_12 > > I would have expected that the various versions would select this, > i.e. something like: > > config BR2_PACKAGE_GDB_NEEDS_CXX11 > bool > > > config BR2_GDB_VERSION_7_10 > bool "7.10" > > config BR2_GDB_VERSION_8_0 > bool "8.0" > select BR2_PACKAGE_GDB_NEEDS_CXX11 But this doesn't work; The BR2_GDB_VERSION_8_0 symbol is only enabled if we build a host variant of gdb. If we build just the target variant, then no version is selected at all, and therefore this symbol would not be selected. So in fact, even my code is slightly wrong: it becomes correct once the default version of gdb is 8.0, but it is wrong when the default version is still 7.12 (which is the case at the time my patch is introduced). Indeed, when my patch is introduced, 7.12 is the default gdb version, used if you build only the target gdb. When you build just the target gdb version, BR2_GDB_VERSION_7_12 is not enabled, and therefore BR2_PACKAGE_GDB_NEEDS_CXX11=y even if it's not true. So I should adjust that. I could use the BR2_GDB_VERSION string option, which exists regardless of whether host gdb is enabled or not. Or I could rely on BR2_PACKAGE_HOST_GDB somehow. Meh. This is crappy :) Thomas
Thomas, All, On 2018-02-07 13:36 +0100, Thomas Petazzoni spake thusly: > On Wed, 7 Feb 2018 10:58:28 +0100, Yann E. MORIN wrote: [--SNIP--] > > > +config BR2_PACKAGE_GDB_NEEDS_CXX11 > > > + bool > > > + default y > > > + depends on !BR2_GDB_VERSION_7_10 > > > + depends on !BR2_GDB_VERSION_7_11 > > > + depends on !BR2_GDB_VERSION_7_12 > > I would have expected that the various versions would select this, > > i.e. something like: > > > > config BR2_PACKAGE_GDB_NEEDS_CXX11 > > bool > > > > > > config BR2_GDB_VERSION_7_10 > > bool "7.10" > > > > config BR2_GDB_VERSION_8_0 > > bool "8.0" > > select BR2_PACKAGE_GDB_NEEDS_CXX11 > > But this doesn't work; The BR2_GDB_VERSION_8_0 symbol is only enabled > if we build a host variant of gdb. If we build just the target > variant, then no version is selected at all, and therefore this symbol > would not be selected. Gah, I missed that... > So in fact, even my code is slightly wrong: it becomes correct once the > default version of gdb is 8.0, but it is wrong when the default version > is still 7.12 (which is the case at the time my patch is introduced). > > Indeed, when my patch is introduced, 7.12 is the default gdb version, > used if you build only the target gdb. When you build just the target > gdb version, BR2_GDB_VERSION_7_12 is not enabled, and therefore > BR2_PACKAGE_GDB_NEEDS_CXX11=y even if it's not true. > > So I should adjust that. I could use the BR2_GDB_VERSION string > option, which exists regardless of whether host gdb is enabled or not. > Or I could rely on BR2_PACKAGE_HOST_GDB somehow. > > Meh. This is crappy :) OK, I think I have an idea about all this mess... But since I can have pretty weird ideas, I make no promise that it will materialise into something usefull... Regards, Yann E. MORIN.
On 07-02-18 18:34, Yann E. MORIN wrote: > Thomas, All, > > On 2018-02-07 13:36 +0100, Thomas Petazzoni spake thusly: >> On Wed, 7 Feb 2018 10:58:28 +0100, Yann E. MORIN wrote: > [--SNIP--] >>>> +config BR2_PACKAGE_GDB_NEEDS_CXX11 >>>> + bool >>>> + default y >>>> + depends on !BR2_GDB_VERSION_7_10 >>>> + depends on !BR2_GDB_VERSION_7_11 >>>> + depends on !BR2_GDB_VERSION_7_12 >>> I would have expected that the various versions would select this, >>> i.e. something like: >>> >>> config BR2_PACKAGE_GDB_NEEDS_CXX11 >>> bool >>> >>> >>> config BR2_GDB_VERSION_7_10 >>> bool "7.10" >>> >>> config BR2_GDB_VERSION_8_0 >>> bool "8.0" >>> select BR2_PACKAGE_GDB_NEEDS_CXX11 >> >> But this doesn't work; The BR2_GDB_VERSION_8_0 symbol is only enabled >> if we build a host variant of gdb. If we build just the target >> variant, then no version is selected at all, and therefore this symbol >> would not be selected. > > Gah, I missed that... > >> So in fact, even my code is slightly wrong: it becomes correct once the >> default version of gdb is 8.0, but it is wrong when the default version >> is still 7.12 (which is the case at the time my patch is introduced). >> >> Indeed, when my patch is introduced, 7.12 is the default gdb version, >> used if you build only the target gdb. When you build just the target >> gdb version, BR2_GDB_VERSION_7_12 is not enabled, and therefore >> BR2_PACKAGE_GDB_NEEDS_CXX11=y even if it's not true. >> >> So I should adjust that. I could use the BR2_GDB_VERSION string >> option, which exists regardless of whether host gdb is enabled or not. >> Or I could rely on BR2_PACKAGE_HOST_GDB somehow. >> >> Meh. This is crappy :) > > OK, I think I have an idea about all this mess... But since I can have > pretty weird ideas, I make no promise that it will materialise into > something usefull... Given this mess, let's ask the question: why do we need version selection to begin with? For GCC I can still see it (it breaks things), for binutils maybe, but gdb? Has been pretty stable lately, no? That said, the C++11 support is obviously problematic. Since we anyway have the versioned gdb infrastructure in place now, I would remove the user-visible choice, but make an automatic choice based on the availability of C++11. It still is a little tricky to express, since we want to use 7.12 when (BR2_PACKAGE_GDB && !(BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 && BR2_INSTALL_LIBSTDCPP) || (BR2_PACKAGE_HOST_GDB && !BR2_HOST_GCC_AT_LEAST_4_8). Or we would have to allow varying the gdb version on host and target (which by the way *should* be OK, but may not be...). So I messed around with gdb a bit, I noticed a couple of things. - gdbserver really needs C++11; some of the shared files in the common directory are C++ files. - host-gdb does not build gdbserver. So all the dependencies we have in host-gdb on the target's toolchain are phony. Or perhaps they are there just in case target gdb is selected as well, but then it's not expressed very well... - With the above in mind, the depends on !BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY in host-gdb is definitely phony. Perhaps it should be in target gdb? Bottom line, I think could be simplified a lot here by removing the version choice. Regards, Arnout
Hello, On Thu, 8 Feb 2018 01:22:20 +0100, Arnout Vandecappelle wrote: > So I messed around with gdb a bit, I noticed a couple of things. > > - gdbserver really needs C++11; some of the shared files in the common directory > are C++ files. Correct. > - host-gdb does not build gdbserver. So all the dependencies we have in host-gdb > on the target's toolchain are phony. Or perhaps they are there just in case > target gdb is selected as well, but then it's not expressed very well... Those target toolchain dependencies in the host-gdb package where here to avoid selecting a wrong version of host-gdb (which affects the target gdb version) when the target toolchain does not have the required feature. For example, we would allow to select 8.0 only if the host toolchain *and* the target toolchain had C++11 support. My PATCH 1/5 in this series is precisely here to rework that. > Bottom line, I think could be simplified a lot here by removing the version choice. Everything can be simplified with less options, it's pretty obvious. The question is whether having those options is useful :-) Thomas
diff --git a/package/gdb/Config.in b/package/gdb/Config.in index af020f40c5..b7f0565b92 100644 --- a/package/gdb/Config.in +++ b/package/gdb/Config.in @@ -11,15 +11,30 @@ comment "gdb/gdbserver needs a toolchain w/ threads, threads debug" depends on BR2_PACKAGE_GDB_ARCH_SUPPORTS depends on !BR2_TOOLCHAIN_HAS_THREADS || !BR2_TOOLCHAIN_HAS_THREADS_DEBUG +config BR2_PACKAGE_GDB_NEEDS_CXX11 + bool + default y + depends on !BR2_GDB_VERSION_7_10 + depends on !BR2_GDB_VERSION_7_11 + depends on !BR2_GDB_VERSION_7_12 + +comment "gdb/gdbserver >= 8.x needs a toolchain w/ C++, gcc >= 4.8" + depends on BR2_PACKAGE_GDB_NEEDS_CXX11 + depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 + config BR2_PACKAGE_GDB bool "gdb" depends on BR2_TOOLCHAIN_HAS_THREADS && BR2_TOOLCHAIN_HAS_THREADS_DEBUG depends on BR2_PACKAGE_GDB_ARCH_SUPPORTS + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_PACKAGE_GDB_NEEDS_CXX11 + depends on BR2_INSTALL_LIBSTDCPP || !BR2_PACKAGE_GDB_NEEDS_CXX11 # When the external toolchain gdbserver is copied to the # target, we don't allow building a separate gdbserver. The # one from the external toolchain should be used. select BR2_PACKAGE_GDB_SERVER if \ (!BR2_PACKAGE_GDB_DEBUGGER && !BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY) + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_PACKAGE_GDB_NEEDS_CXX11 + depends on BR2_INSTALL_LIBSTDCPP || !BR2_PACKAGE_GDB_NEEDS_CXX11 help GDB, the GNU Project debugger, allows you to see what is going on `inside' another program while it executes -- or diff --git a/package/gdb/Config.in.host b/package/gdb/Config.in.host index 99e1cae5ba..8676c6f01b 100644 --- a/package/gdb/Config.in.host +++ b/package/gdb/Config.in.host @@ -3,7 +3,6 @@ config BR2_PACKAGE_HOST_GDB_ARCH_SUPPORTS default y # The ARC version needs C++11, thus gcc >= 4.8, like gdb-8.0.x depends on BR2_HOST_GCC_AT_LEAST_4_8 || !BR2_arc - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || !BR2_arc depends on !((BR2_arm || BR2_armeb) && BR2_BINFMT_FLAT) depends on !BR2_microblaze depends on !BR2_nios2 @@ -63,9 +62,7 @@ config BR2_GDB_VERSION_7_12 config BR2_GDB_VERSION_8_0 bool "gdb 8.0.x" # Needs a C++11 compiler - depends on BR2_INSTALL_LIBSTDCPP depends on BR2_HOST_GCC_AT_LEAST_4_8 - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 endchoice
As we are about to switch to 8.0 as the default gdb dependency, we need to adjust how the gdb dependencies are handled. Indeed, from 8.0 onwards, gdb needs a C++11 capable compiler, i.e at least gcc 4.8. Until now, Config.in.host was making sure that gdb 8.0 was not selectable if the cross-compilation toolchain did not have C++ support with gcc >= 4.8. This worked fine because the default version of gdb, used as the target gdb version when no host gdb is built, was 7.11, and did not require C++11. With the switch to 8.0 as the default version, when target gdb is enabled but not host gdb, 8.0 is used, which means we need a C++11 capable compiler. The dependencies in Config.in.host are no longer sufficient. So instead, we remove the target-related dependencies from Config.in.host and move them properly to Config.in. The overall logic is the following: - In Config.in.host, BR2_PACKAGE_HOST_GDB_ARCH_SUPPORTS ensures that we have at least host gcc 4.8 if we're on ARC, because the ARC gdb needs C++11. We remove the target toolchain related dependencies from here. - In Config.in.host, the version selection ensures that 8.0 cannot be selected if the host toolchain does not have at least gcc 4.8. We remove the target toolchain related dependencies from here. - In Config.in, we handle gdb more like a regular target package, with a Config.in comment when its dependencies are not met. A hidden boolean BR2_PACKAGE_GDB_NEEDS_CXX11 indicates when the currently selected version requires C++11. Based on that, we show the Config.in comment, and add the proper dependencies to BR2_PACKAGE_GDB. It is worth mentioning that BR2_PACKAGE_GDB_NEEDS_CXX11 is intentionally created to be !7.10 && !7.11 && !7.12 instead of 8.0, because we will gradually add more C++11-requiring versions, and remove non-C++11-requiring versions. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- package/gdb/Config.in | 15 +++++++++++++++ package/gdb/Config.in.host | 3 --- 2 files changed, 15 insertions(+), 3 deletions(-)