[v2,1/5] package/gdb: rework dependency for C++11

Message ID 20180205211015.26819-2-thomas.petazzoni@bootlin.com
State New
Headers show
Series
  • Add gdb 8.1 support, switch to 8.0 as default, drop old versions
Related show

Commit Message

Thomas Petazzoni Feb. 5, 2018, 9:10 p.m.
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(-)

Comments

Yann E. MORIN Feb. 7, 2018, 9:58 a.m. | #1
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
>
Thomas Petazzoni Feb. 7, 2018, 12:36 p.m. | #2
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
Yann E. MORIN Feb. 7, 2018, 5:34 p.m. | #3
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.
Arnout Vandecappelle Feb. 8, 2018, 12:22 a.m. | #4
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
Thomas Petazzoni Feb. 8, 2018, 8:05 a.m. | #5
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

Patch

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