[PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Christopher Di Bella]

`std::back_inserter` is defined in `<iterator>`, which is currently
being transitively included by one of the other headers. This is causing
gdb to fail to build on certain platforms, which is fixed by explicitly
including it.
---
 gdb/value.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/value.c b/gdb/value.c
index 022fca91a42..ba7ae1a0e18 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -40,6 +40,7 @@
 #include "cp-abi.h"
 #include "user-regs.h"
 #include <algorithm>
+#include <iterator>
 #include "completer.h"
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/array-view.h"
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Enze Li]

On Tue, 2022-06-28 at 01:04 +0000, Christopher Di Bella via Gdb-patches
wrote:
> `std::back_inserter` is defined in `<iterator>`, which is currently
> being transitively included by one of the other headers. This is
> causing
> gdb to fail to build on certain platforms, which is fixed by
> explicitly
> including it.

Hi Christopher,

I didn't get your point through the commit message.  Can you clarify
exactly on which platforms, and what build error you encountered?  So
that I can reproduce the issue.

Thanks,
Enze


> ---
>  gdb/value.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/value.c b/gdb/value.c
> index 022fca91a42..ba7ae1a0e18 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -40,6 +40,7 @@
>  #include "cp-abi.h"
>  #include "user-regs.h"
>  #include <algorithm>
> +#include <iterator>
>  #include "completer.h"
>  #include "gdbsupport/selftest.h"
>  #include "gdbsupport/array-view.h"

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Christopher Di Bella]

Hi Enze,

`std::back_inserter` is defined in <iterator>, which isn't included in
`gdb/value.c`. Because the C preprocessor imports headers by copy/pasting
the contents at the `#include` site, this means that another standard
library header is exposing it (presumably `<algorithm>`).

We experienced this on ChromeOS, which uses Clang and libc++ to build
things.

```
/tmp/portage/sys-devel/gdb-9.2.20200923-r7/work/gdb-9.2/gdb/value.c:1648:52:
error: no member named 'back_inserter' in namespace 'std'
       std::move (iter + 1, all_values.end (), std::back_inserter (result));
                                               ~~~~~^
 1 error generated.
```

On Fri, 1 Jul 2022 at 06:45, Enze Li <enze.li@hotmail.com> wrote:

> On Tue, 2022-06-28 at 01:04 +0000, Christopher Di Bella via Gdb-patches
> wrote:
> > `std::back_inserter` is defined in `<iterator>`, which is currently
> > being transitively included by one of the other headers. This is
> > causing
> > gdb to fail to build on certain platforms, which is fixed by
> > explicitly
> > including it.
>
> Hi Christopher,
>
> I didn't get your point through the commit message.  Can you clarify
> exactly on which platforms, and what build error you encountered?  So
> that I can reproduce the issue.
>
> Thanks,
> Enze
>
>
> > ---
> >  gdb/value.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/gdb/value.c b/gdb/value.c
> > index 022fca91a42..ba7ae1a0e18 100644
> > --- a/gdb/value.c
> > +++ b/gdb/value.c
> > @@ -40,6 +40,7 @@
> >  #include "cp-abi.h"
> >  #include "user-regs.h"
> >  #include <algorithm>
> > +#include <iterator>
> >  #include "completer.h"
> >  #include "gdbsupport/selftest.h"
> >  #include "gdbsupport/array-view.h"
>
>

-- 
Kind regards,

Christopher Di Bella (he/him/his)

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Enze Li]

Hi Christopher,

Unfortunately, since I don't have a ChromeBook, I can not reproduce the
problem you encountered.

I tried to reproduce your problem on a x86_64-linux device and still
did not reproduce it with the trunk.  After digging into this deeply, I
found that 'gdb/value.c' has already includes the 'vector' header file
after preprocessing.

Here's what I got,

value.c
  |--defs.h
      |--common-defs.h
          |--common-utils.h (#include <vector>)

Finally, I noticed that you were compiling gdb-9.2.  I know nothing
about the maintenance of closed branch.  You may want to keep PING and
maintainers would show up to look at this.

Thanks,
Enze

On Fri, 2022-07-01 at 14:50 -0700, Christopher Di Bella wrote:
> Hi Enze,
> 
> `std::back_inserter` is defined in <iterator>, which isn't included
> in `gdb/value.c`. Because the C preprocessor imports headers by
> copy/pasting the contents at the `#include` site, this means that
> another standard library header is exposing it (presumably
> `<algorithm>`).
> 
> We experienced this on ChromeOS, which uses Clang and libc++ to build
> things.
> 
> ```
> /tmp/portage/sys-devel/gdb-9.2.20200923-r7/work/gdb-
> 9.2/gdb/value.c:1648:52: error: no member named 'back_inserter' in
> namespace 'std'
>        std::move (iter + 1, all_values.end (), std::back_inserter
> (result));
>                                                ~~~~~^
>  1 error generated.
> ```
> 
> On Fri, 1 Jul 2022 at 06:45, Enze Li <enze.li@hotmail.com> wrote:
> > On Tue, 2022-06-28 at 01:04 +0000, Christopher Di Bella via Gdb-
> > patches
> > wrote:
> > > `std::back_inserter` is defined in `<iterator>`, which is
> > > currently
> > > being transitively included by one of the other headers. This is
> > > causing
> > > gdb to fail to build on certain platforms, which is fixed by
> > > explicitly
> > > including it.
> > 
> > Hi Christopher,
> > 
> > I didn't get your point through the commit message.  Can you
> > clarify
> > exactly on which platforms, and what build error you encountered? 
> > So
> > that I can reproduce the issue.
> > 
> > Thanks,
> > Enze
> > 
> > 
> > > ---
> > >  gdb/value.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/gdb/value.c b/gdb/value.c
> > > index 022fca91a42..ba7ae1a0e18 100644
> > > --- a/gdb/value.c
> > > +++ b/gdb/value.c
> > > @@ -40,6 +40,7 @@
> > >  #include "cp-abi.h"
> > >  #include "user-regs.h"
> > >  #include <algorithm>
> > > +#include <iterator>
> > >  #include "completer.h"
> > >  #include "gdbsupport/selftest.h"
> > >  #include "gdbsupport/array-view.h"
> > 
> 
> 

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Manoj Gupta]

I work on ChromeOS and can provide answers to this.

<vector> does not include provide std::back_inserter as per C++ spec. It is
provided by <iterator> [1].
The fact that it worked so far is just an accident that libc++ and
libstdc++ were including this
header with <vector>.
With libc++, it is no longer true. See commit "[libc++] Removes unneeded
<iterator> includes." in libc++ [2].
It is therefore not surprising that gdb fails to build.


On Fri, Jul 8, 2022 at 9:30 PM Enze Li <enze.li@hotmail.com> wrote:

> Hi Christopher,
>
> Unfortunately, since I don't have a ChromeBook, I can not reproduce the
> problem you encountered.
>
> I tried to reproduce your problem on a x86_64-linux device and still
> did not reproduce it with the trunk.  After digging into this deeply, I
> found that 'gdb/value.c' has already includes the 'vector' header file
> after preprocessing.
>
> Here's what I got,
>
> value.c
>   |--defs.h
>       |--common-defs.h
>           |--common-utils.h (#include <vector>)
>
> Finally, I noticed that you were compiling gdb-9.2.  I know nothing
> about the maintenance of closed branch.  You may want to keep PING and
> maintainers would show up to look at this.
>
> Inclusion of <vector> is not guaranteed to provide <iterator>. And I
believe that the current gdb still does not include <iterator> in value.c
[3].
So I am sure that this patch is still needed in trunk.

Note: Looking at value.c, I think it needs more header includes e.g.
std::move [4] is used but <utility> is not included.

[1] https://en.cppreference.com/w/cpp/iterator/back_inserter
[2] [libc++] Removes unneeded <iterator> includes:
https://reviews.llvm.org/rG4cd04d1687f1096990119304a3eb22081ab4bb29
[3]
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/value.c;h=022fca91a42a8b4d5bccc745b62e642aea8a02ed;hb=refs/heads/master
[4]: https://en.cppreference.com/w/cpp/utility/move

Thanks,
Manoj



> Thanks,
> Enze
>
> On Fri, 2022-07-01 at 14:50 -0700, Christopher Di Bella wrote:
> > Hi Enze,
> >
> > `std::back_inserter` is defined in <iterator>, which isn't included
> > in `gdb/value.c`. Because the C preprocessor imports headers by
> > copy/pasting the contents at the `#include` site, this means that
> > another standard library header is exposing it (presumably
> > `<algorithm>`).
> >
> > We experienced this on ChromeOS, which uses Clang and libc++ to build
> > things.
> >
> > ```
> > /tmp/portage/sys-devel/gdb-9.2.20200923-r7/work/gdb-
> > 9.2/gdb/value.c:1648:52: error: no member named 'back_inserter' in
> > namespace 'std'
> >        std::move (iter + 1, all_values.end (), std::back_inserter
> > (result));
> >                                                ~~~~~^
> >  1 error generated.
> > ```
> >
> > On Fri, 1 Jul 2022 at 06:45, Enze Li <enze.li@hotmail.com> wrote:
> > > On Tue, 2022-06-28 at 01:04 +0000, Christopher Di Bella via Gdb-
> > > patches
> > > wrote:
> > > > `std::back_inserter` is defined in `<iterator>`, which is
> > > > currently
> > > > being transitively included by one of the other headers. This is
> > > > causing
> > > > gdb to fail to build on certain platforms, which is fixed by
> > > > explicitly
> > > > including it.
> > >
> > > Hi Christopher,
> > >
> > > I didn't get your point through the commit message.  Can you
> > > clarify
> > > exactly on which platforms, and what build error you encountered?
> > > So
> > > that I can reproduce the issue.
> > >
> > > Thanks,
> > > Enze
> > >
> > >
> > > > ---
> > > >  gdb/value.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/gdb/value.c b/gdb/value.c
> > > > index 022fca91a42..ba7ae1a0e18 100644
> > > > --- a/gdb/value.c
> > > > +++ b/gdb/value.c
> > > > @@ -40,6 +40,7 @@
> > > >  #include "cp-abi.h"
> > > >  #include "user-regs.h"
> > > >  #include <algorithm>
> > > > +#include <iterator>
> > > >  #include "completer.h"
> > > >  #include "gdbsupport/selftest.h"
> > > >  #include "gdbsupport/array-view.h"
> > >
> >
> >
>
>

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Enze Li]

Hi Manoj,

On Fri, 2022-07-08 at 22:57 -0700, Manoj Gupta wrote:
> I work on ChromeOS and can provide answers to this.
> 
> <vector> does not include provide std::back_inserter as per C++ spec.
> It is provided by <iterator> [1].
> The fact that it worked so far is just an accident that libc++ and
> libstdc++ were including this
> header with <vector>.
> With libc++, it is no longer true. See commit "[libc++] Removes
> unneeded <iterator> includes." in libc++ [2].
> It is therefore not surprising that gdb fails to build. 

Thank you for the detailed explanation.  It seems that we need to
manually compile and install the libc++ from upstream before we can
reproduce this issue.

> 
> 
> On Fri, Jul 8, 2022 at 9:30 PM Enze Li <enze.li@hotmail.com> wrote:
> > Hi Christopher,
> > 
> > Unfortunately, since I don't have a ChromeBook, I can not reproduce
> > the
> > problem you encountered.
> > 
> > I tried to reproduce your problem on a x86_64-linux device and
> > still
> > did not reproduce it with the trunk.  After digging into this
> > deeply, I
> > found that 'gdb/value.c' has already includes the 'vector' header
> > file
> > after preprocessing.
> > 
> > Here's what I got,
> > 
> > value.c
> >   |--defs.h
> >       |--common-defs.h
> >           |--common-utils.h (#include <vector>)

Geez.  I had mistakenly read that we needed to include 'vector' header
file.  Please ignore this part. It must be an oversight. 😂️

Thanks,
Enze

> > 
> > Finally, I noticed that you were compiling gdb-9.2.  I know nothing
> > about the maintenance of closed branch.  You may want to keep PING
> > and
> > maintainers would show up to look at this.
> > 
> 
> Inclusion of <vector> is not guaranteed to provide <iterator>. And I
> believe that the current gdb still does not include <iterator> in
> value.c [3].
> So I am sure that this patch is still needed in trunk.
> 
> Note: Looking at value.c, I think it needs more header includes e.g.
> std::move [4] is used but <utility> is not included.
> 
> [1] https://en.cppreference.com/w/cpp/iterator/back_inserter
> [2] [libc++] Removes unneeded <iterator>
> includes:  https://reviews.llvm.org/rG4cd04d1687f1096990119304a3eb220
> 81ab4bb29
> [3]
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/value.c;h=022fca91a42a8b4d5bccc745b62e642aea8a02ed;hb=refs/heads/master
> [4]: https://en.cppreference.com/w/cpp/utility/move
> 
> Thanks,
> Manoj 
> 
>  
> > Thanks,
> > Enze
> > 
> > On Fri, 2022-07-01 at 14:50 -0700, Christopher Di Bella wrote:
> > > Hi Enze,
> > > 
> > > `std::back_inserter` is defined in <iterator>, which isn't
> > > included
> > > in `gdb/value.c`. Because the C preprocessor imports headers by
> > > copy/pasting the contents at the `#include` site, this means that
> > > another standard library header is exposing it (presumably
> > > `<algorithm>`).
> > > 
> > > We experienced this on ChromeOS, which uses Clang and libc++ to
> > > build
> > > things.
> > > 
> > > ```
> > > /tmp/portage/sys-devel/gdb-9.2.20200923-r7/work/gdb-
> > > 9.2/gdb/value.c:1648:52: error: no member named 'back_inserter'
> > > in
> > > namespace 'std'
> > >        std::move (iter + 1, all_values.end (), std::back_inserter
> > > (result));
> > >                                                ~~~~~^
> > >  1 error generated.
> > > ```
> > > 
> > > On Fri, 1 Jul 2022 at 06:45, Enze Li <enze.li@hotmail.com> wrote:
> > > > On Tue, 2022-06-28 at 01:04 +0000, Christopher Di Bella via
> > > > Gdb-
> > > > patches
> > > > wrote:
> > > > > `std::back_inserter` is defined in `<iterator>`, which is
> > > > > currently
> > > > > being transitively included by one of the other headers. This
> > > > > is
> > > > > causing
> > > > > gdb to fail to build on certain platforms, which is fixed by
> > > > > explicitly
> > > > > including it.
> > > > 
> > > > Hi Christopher,
> > > > 
> > > > I didn't get your point through the commit message.  Can you
> > > > clarify
> > > > exactly on which platforms, and what build error you
> > > > encountered? 
> > > > So
> > > > that I can reproduce the issue.
> > > > 
> > > > Thanks,
> > > > Enze
> > > > 
> > > > 
> > > > > ---
> > > > >  gdb/value.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/gdb/value.c b/gdb/value.c
> > > > > index 022fca91a42..ba7ae1a0e18 100644
> > > > > --- a/gdb/value.c
> > > > > +++ b/gdb/value.c
> > > > > @@ -40,6 +40,7 @@
> > > > >  #include "cp-abi.h"
> > > > >  #include "user-regs.h"
> > > > >  #include <algorithm>
> > > > > +#include <iterator>
> > > > >  #include "completer.h"
> > > > >  #include "gdbsupport/selftest.h"
> > > > >  #include "gdbsupport/array-view.h"
> > > > 
> > > 
> > > 
> > 

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Andrew Burgess]

* Christopher Di Bella via Gdb-patches <gdb-patches@sourceware.org> [2022-06-28 01:04:27 +0000]:

> `std::back_inserter` is defined in `<iterator>`, which is currently
> being transitively included by one of the other headers. This is causing
> gdb to fail to build on certain platforms, which is fixed by explicitly
> including it.

Hi Christopher,

Thanks for this fix.  If you could update the commit message with the
additional information that Manoj provided then repost, and we can get
this merged.

Thanks,
Andrew



> ---
>  gdb/value.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/value.c b/gdb/value.c
> index 022fca91a42..ba7ae1a0e18 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -40,6 +40,7 @@
>  #include "cp-abi.h"
>  #include "user-regs.h"
>  #include <algorithm>
> +#include <iterator>
>  #include "completer.h"
>  #include "gdbsupport/selftest.h"
>  #include "gdbsupport/array-view.h"
> -- 
> 2.37.0.rc0.161.g10f37bed90-goog
> 

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Simon Marchi]

On 2022-07-09 04:54, Enze Li via Gdb-patches wrote:
> Hi Manoj,
> 
> On Fri, 2022-07-08 at 22:57 -0700, Manoj Gupta wrote:
>> I work on ChromeOS and can provide answers to this.
>>
>> <vector> does not include provide std::back_inserter as per C++ spec.
>> It is provided by <iterator> [1].
>> The fact that it worked so far is just an accident that libc++ and
>> libstdc++ were including this
>> header with <vector>.
>> With libc++, it is no longer true. See commit "[libc++] Removes
>> unneeded <iterator> includes." in libc++ [2].
>> It is therefore not surprising that gdb fails to build. 
> 
> Thank you for the detailed explanation.  It seems that we need to
> manually compile and install the libc++ from upstream before we can
> reproduce this issue.

This is trivial enough that it's not really necessary to reproduce to
get convinced that such a change would be good.  The file uses
back_inserter -> C++ says that back_inserter is provided by <iterator>
-> make the file include <iterator>.  This has no real change of
breaking anything.

Simon

Re: [PATCH] [PATCH] [gdb] adds `<iterator>` to list of includes

[Enze Li]

On Sun, 2022-07-10 at 14:52 -0400, Simon Marchi wrote:
> On 2022-07-09 04:54, Enze Li via Gdb-patches wrote:
> > Hi Manoj,
> > 
> > On Fri, 2022-07-08 at 22:57 -0700, Manoj Gupta wrote:
> > > I work on ChromeOS and can provide answers to this.
> > > 
> > > <vector> does not include provide std::back_inserter as per C++
> > > spec.
> > > It is provided by <iterator> [1].
> > > The fact that it worked so far is just an accident that libc++
> > > and
> > > libstdc++ were including this
> > > header with <vector>.
> > > With libc++, it is no longer true. See commit "[libc++] Removes
> > > unneeded <iterator> includes." in libc++ [2].
> > > It is therefore not surprising that gdb fails to build. 
> > 
> > Thank you for the detailed explanation.  It seems that we need to
> > manually compile and install the libc++ from upstream before we can
> > reproduce this issue.
> 
> This is trivial enough that it's not really necessary to reproduce to
> get convinced that such a change would be good.  The file uses
> back_inserter -> C++ says that back_inserter is provided by
> <iterator>
> -> make the file include <iterator>.  This has no real change of
> breaking anything.

Indeed, reproduction is not necessary.  I was just curious about
building gdb with libc++.  I hope my interruptions didn't interfere
with the progress of reviewing the patch.

Best Regards,
Enze

> 
> Simon