Implement IPv6 support for GDB/gdbserver
Commit Message
This patch implements IPv6 support for both GDB and gdbserver. Based
on my research, it is the fourth attempt to do that since 2006. Since
I used ideas from all of the previous patches, I also added their
authors's names on the ChangeLogs as a way to recognize their
efforts. For reference sake, you can find the previous attempts at:
https://sourceware.org/ml/gdb-patches/2006-09/msg00192.html
https://sourceware.org/ml/gdb-patches/2014-02/msg00248.html
https://sourceware.org/ml/gdb-patches/2016-02/msg00226.html
The basic idea behind the patch is to start using the new
'getaddrinfo'/'getnameinfo' calls, which are responsible for
translating names and addresses in a protocol-independent way. This
means that if we ever have an IPv8, we won't need to change the code
again.
The function 'getaddrinfo' returns a linked list of possible addresses
to connect to, so I modified ser-tcp.c:net_open's code to loop through
the linked list and try all the addresses until it finds a valid one.
The same rationale was used for gdbserver, but without the "retry"
mechanism that GDB has.
I also implemented some hostname parsing functions which are used to
help GDB and gdbserver to parse hostname strings provided by the user.
These new functions are living inside common/netstuff.[ch]. I've had
to do that since IPv6 introduces a new URL scheme, which defines that
square brackets can be used to enclose the host part and differentiate
it from the port (e.g., "[::1]:1234" means "host ::1, port 1234"). I
spent some time thinking about a reasonable way to interpret what the
user wants, and I came up with the following:
- If the user has provided a prefix that doesn't specify the protocol
version (i.e., "tcp:" or "udp:"), or if the user has not provided
any prefix, don't make any assumptions (i.e., assume AF_UNSPEC when
dealing with 'getaddrinfo') *unless* the host starts with "[" (in
which case, assume it's an IPv6 host).
- If the user has provided a prefix that does specify the protocol
version (i.e., "tcp4:", "tcp6:", "udp4:" or "udp6:"), then respect
that.
This method doesn't follow strictly what RFC 2732 proposes (that
literal IPv6 addresses should be provided enclosed in "[" and "]")
because IPv6 addresses still can be provided without square brackets
in our case, but since we have prefixes to specify protocol versions I
think this is not an issue.
Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
parameter, which instructs GDB and gdbserver to use IPv6 for
connections. This way, if you want to run IPv6 tests, you do:
$ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
This required a few changes on the gdbserver-base.exp,
native-gdbserver.exp and native-extended-gdbserver.exp boards, and
also a minimal adjustment on gdb.server/run-without-local-binary.exp.
This patch has been regression-tested on BuildBot and locally, and
also built using a x86_64-w64-mingw32 GCC, and no problems were found.
gdb/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
Jan Kratochvil <jan.kratochvil@redhat.com>
Paul Fertser <fercerpav@gmail.com>
Tsutomu Seki <sekiriki@gmail.com>
* Makefile.in (COMMON_SFILES): Add 'common/netstuff.c'.
(HFILES_NO_SRCDIR): Add 'common/netstuff.h'.
* NEWS (Changes since GDB 8.1): Mention IPv6 support.
* common/netstuff.c: New file.
* common/netstuff.h: New file.
* ser-tcp.c: Include 'netstuff.h' and 'wspiapi.h'.
(net_open): Handle IPv6-style hostnames; implement support for
IPv6 connections.
gdb/gdbserver/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
Jan Kratochvil <jan.kratochvil@redhat.com>
Paul Fertser <fercerpav@gmail.com>
Tsutomu Seki <sekiriki@gmail.com>
* Makefile.in (SFILES): Add '$(srcdir)/common/netstuff.c'.
(OBS): Add 'common/netstuff.o'.
* gdbreplay.c: Include 'wspiapi.h'.
(remote_open): Implement support for IPv6
connections.
* remote-utils.c: Include 'netstuff.h', 'filestuff.h'
and 'wspiapi.h'.
(handle_accept_event): Accept connections from IPv6 sources.
(remote_prepare): Handle IPv6-style hostnames; implement
support for IPv6 connections.
(remote_open): Implement support for printing connections from
IPv6 sources.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
Jan Kratochvil <jan.kratochvil@redhat.com>
Paul Fertser <fercerpav@gmail.com>
Tsutomu Seki <sekiriki@gmail.com>
* README (Testsuite Parameters): Mention new 'GDB_TEST_IPV6'
parameter.
* boards/gdbserver-base.exp (get_comm_port_localhost_ipv6):
New procedure.
* boards/native-extended-gdbserver.exp: Detect 'GDB_TEST_IPV6'
and change board info accordingly.
* boards/native-gdbserver.exp: Likewise.
* gdb.server/run-without-local-binary.exp: Improve regexp used
for detecting when a remote debugging connection succeeds.
gdb/doc/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
Jan Kratochvil <jan.kratochvil@redhat.com>
Paul Fertser <fercerpav@gmail.com>
Tsutomu Seki <sekiriki@gmail.com>
* gdb.texinfo (Remote Connection Commands): Add explanation
about new IPv6 support. Add new connection prefixes.
---
gdb/Makefile.in | 2 +
gdb/NEWS | 4 +
gdb/common/netstuff.c | 136 +++++++++++++
gdb/common/netstuff.h | 52 +++++
gdb/doc/gdb.texinfo | 48 ++++-
gdb/gdbserver/Makefile.in | 2 +
gdb/gdbserver/gdbreplay.c | 181 +++++++++++++----
gdb/gdbserver/remote-utils.c | 119 +++++++----
gdb/ser-tcp.c | 217 ++++++++++-----------
gdb/testsuite/README | 7 +
gdb/testsuite/boards/gdbserver-base.exp | 5 +
gdb/testsuite/boards/native-extended-gdbserver.exp | 7 +-
gdb/testsuite/boards/native-gdbserver.exp | 7 +-
.../gdb.server/run-without-local-binary.exp | 2 +-
14 files changed, 602 insertions(+), 187 deletions(-)
create mode 100644 gdb/common/netstuff.c
create mode 100644 gdb/common/netstuff.h
Comments
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>,
> Eli Zaretskii <eliz@gnu.org>,
> Jan Kratochvil <jan.kratochvil@redhat.com>,
> Paul Fertser <fercerpav@gmail.com>,
> Tsutomu Seki <sekiriki@gmail.com>,
> Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Wed, 23 May 2018 14:57:19 -0400
>
> gdb/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * Makefile.in (COMMON_SFILES): Add 'common/netstuff.c'.
> (HFILES_NO_SRCDIR): Add 'common/netstuff.h'.
> * NEWS (Changes since GDB 8.1): Mention IPv6 support.
> * common/netstuff.c: New file.
> * common/netstuff.h: New file.
> * ser-tcp.c: Include 'netstuff.h' and 'wspiapi.h'.
> (net_open): Handle IPv6-style hostnames; implement support for
> IPv6 connections.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * Makefile.in (SFILES): Add '$(srcdir)/common/netstuff.c'.
> (OBS): Add 'common/netstuff.o'.
> * gdbreplay.c: Include 'wspiapi.h'.
> (remote_open): Implement support for IPv6
> connections.
> * remote-utils.c: Include 'netstuff.h', 'filestuff.h'
> and 'wspiapi.h'.
> (handle_accept_event): Accept connections from IPv6 sources.
> (remote_prepare): Handle IPv6-style hostnames; implement
> support for IPv6 connections.
> (remote_open): Implement support for printing connections from
> IPv6 sources.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * README (Testsuite Parameters): Mention new 'GDB_TEST_IPV6'
> parameter.
> * boards/gdbserver-base.exp (get_comm_port_localhost_ipv6):
> New procedure.
> * boards/native-extended-gdbserver.exp: Detect 'GDB_TEST_IPV6'
> and change board info accordingly.
> * boards/native-gdbserver.exp: Likewise.
> * gdb.server/run-without-local-binary.exp: Improve regexp used
> for detecting when a remote debugging connection succeeds.
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * gdb.texinfo (Remote Connection Commands): Add explanation
> about new IPv6 support. Add new connection prefixes.
The documentation parts are approved, with a couple of minor comments:
> @item target remote @code{@var{host}:@var{port}}
> +@itemx target remote @code{@var{@r{[}host@r{]}}:@var{port}}
Isn't having 2 lines here redundant? If the HOST part is optional,
the second line already covers the first, right?
Similarly with other forms of this command.
> @item target remote @code{udp:@var{host}:@var{port}}
> +@itemx target remote @code{udp:@var{host}:@var{port}}
And here these two lines are exactly identical, right?
> +To connect to port 2828 on a terminal server whose address is
> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
> +
> +@smallexample
> +target remote [2001::f8ff::67cf]:2828
> +@end smallexample
> +
> +Or explicitly specify the @acronym{IPv6} protocol:
The last sentence is actually a continuation of the one before the
example. So it should start with a lower-case letter, and please
insert a @noindent (on its own line) before that, to prevent Texnfo
processors from indenting that line.
Thanks.
On Wednesday, May 23 2018, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Pedro Alves <palves@redhat.com>,
>> Eli Zaretskii <eliz@gnu.org>,
>> Jan Kratochvil <jan.kratochvil@redhat.com>,
>> Paul Fertser <fercerpav@gmail.com>,
>> Tsutomu Seki <sekiriki@gmail.com>,
>> Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Wed, 23 May 2018 14:57:19 -0400
>>
>> gdb/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>> Paul Fertser <fercerpav@gmail.com>
>> Tsutomu Seki <sekiriki@gmail.com>
>>
>> * Makefile.in (COMMON_SFILES): Add 'common/netstuff.c'.
>> (HFILES_NO_SRCDIR): Add 'common/netstuff.h'.
>> * NEWS (Changes since GDB 8.1): Mention IPv6 support.
>> * common/netstuff.c: New file.
>> * common/netstuff.h: New file.
>> * ser-tcp.c: Include 'netstuff.h' and 'wspiapi.h'.
>> (net_open): Handle IPv6-style hostnames; implement support for
>> IPv6 connections.
>>
>> gdb/gdbserver/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>> Paul Fertser <fercerpav@gmail.com>
>> Tsutomu Seki <sekiriki@gmail.com>
>>
>> * Makefile.in (SFILES): Add '$(srcdir)/common/netstuff.c'.
>> (OBS): Add 'common/netstuff.o'.
>> * gdbreplay.c: Include 'wspiapi.h'.
>> (remote_open): Implement support for IPv6
>> connections.
>> * remote-utils.c: Include 'netstuff.h', 'filestuff.h'
>> and 'wspiapi.h'.
>> (handle_accept_event): Accept connections from IPv6 sources.
>> (remote_prepare): Handle IPv6-style hostnames; implement
>> support for IPv6 connections.
>> (remote_open): Implement support for printing connections from
>> IPv6 sources.
>>
>> gdb/testsuite/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>> Paul Fertser <fercerpav@gmail.com>
>> Tsutomu Seki <sekiriki@gmail.com>
>>
>> * README (Testsuite Parameters): Mention new 'GDB_TEST_IPV6'
>> parameter.
>> * boards/gdbserver-base.exp (get_comm_port_localhost_ipv6):
>> New procedure.
>> * boards/native-extended-gdbserver.exp: Detect 'GDB_TEST_IPV6'
>> and change board info accordingly.
>> * boards/native-gdbserver.exp: Likewise.
>> * gdb.server/run-without-local-binary.exp: Improve regexp used
>> for detecting when a remote debugging connection succeeds.
>>
>> gdb/doc/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>> Paul Fertser <fercerpav@gmail.com>
>> Tsutomu Seki <sekiriki@gmail.com>
>>
>> * gdb.texinfo (Remote Connection Commands): Add explanation
>> about new IPv6 support. Add new connection prefixes.
>
> The documentation parts are approved, with a couple of minor comments:
Thanks for the review, Eli.
>> @item target remote @code{@var{host}:@var{port}}
>> +@itemx target remote @code{@var{@r{[}host@r{]}}:@var{port}}
>
> Isn't having 2 lines here redundant? If the HOST part is optional,
> the second line already covers the first, right?
>
> Similarly with other forms of this command.
Hm, I think you have misunderstood what the square brackets mean, which
tells me that there must be a better way to write this...
The square brackets in this case don't mean that the HOST is optional.
Rather, they *enclose* the hostname. As explained in the text above
this, IPv6 introduced a new way to specify URLs: by enclosing them in
square brackets. This is because the IPv6 separator (':') is the same
as the resource (port) separator, which can cause confusion. Therefore,
an IPv6 URL can have the form:
[::1]:1234
That's what I tried to convey with the additional @itemx. Perhaps I
shouldn't use @r{[} and @r{]}? Not sure if there's a better way to do
that.
>> @item target remote @code{udp:@var{host}:@var{port}}
>> +@itemx target remote @code{udp:@var{host}:@var{port}}
>
> And here these two lines are exactly identical, right?
Indeed. Removed.
>> +To connect to port 2828 on a terminal server whose address is
>> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
>> +
>> +@smallexample
>> +target remote [2001::f8ff::67cf]:2828
>> +@end smallexample
>> +
>> +Or explicitly specify the @acronym{IPv6} protocol:
>
> The last sentence is actually a continuation of the one before the
> example. So it should start with a lower-case letter, and please
> insert a @noindent (on its own line) before that, to prevent Texnfo
> processors from indenting that line.
Ah, OK. Fixed.
Thanks,
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: gdb-patches@sourceware.org, palves@redhat.com, jan.kratochvil@redhat.com, fercerpav@gmail.com, sekiriki@gmail.com
> Date: Wed, 23 May 2018 19:40:06 -0400
>
> The square brackets in this case don't mean that the HOST is optional.
> Rather, they *enclose* the hostname. As explained in the text above
> this, IPv6 introduced a new way to specify URLs: by enclosing them in
> square brackets. This is because the IPv6 separator (':') is the same
> as the resource (port) separator, which can cause confusion. Therefore,
> an IPv6 URL can have the form:
>
> [::1]:1234
Then perhaps we shouldn't advertise the bracket-less syntax at all,
and only say somewhere that it is accepted for backward compatibility?
> Perhaps I shouldn't use @r{[} and @r{]}?
Yes, @r{..} is definitely wrong in that case, you should drop it.
On Thursday, May 24 2018, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: gdb-patches@sourceware.org, palves@redhat.com, jan.kratochvil@redhat.com, fercerpav@gmail.com, sekiriki@gmail.com
>> Date: Wed, 23 May 2018 19:40:06 -0400
>>
>> The square brackets in this case don't mean that the HOST is optional.
>> Rather, they *enclose* the hostname. As explained in the text above
>> this, IPv6 introduced a new way to specify URLs: by enclosing them in
>> square brackets. This is because the IPv6 separator (':') is the same
>> as the resource (port) separator, which can cause confusion. Therefore,
>> an IPv6 URL can have the form:
>>
>> [::1]:1234
>
> Then perhaps we shouldn't advertise the bracket-less syntax at all,
> and only say somewhere that it is accepted for backward compatibility?
Well, the bracket-less syntax is still useful for when you want to
provide IPv4 addresses. For example:
target remote tcp:192.168.1.1:1234
is still a valid use, and:
target remote tcp:[192.168.1.1]:1234
doesn't work/make sense. Therefore, I think it's still important to
mention both syntaxes.
>> Perhaps I shouldn't use @r{[} and @r{]}?
>
> Yes, @r{..} is definitely wrong in that case, you should drop it.
Noted. I'll remove the @r{}.
Thanks,
On Wednesday, May 23 2018, I wrote:
> This patch implements IPv6 support for both GDB and gdbserver. Based
> on my research, it is the fourth attempt to do that since 2006. Since
> I used ideas from all of the previous patches, I also added their
> authors's names on the ChangeLogs as a way to recognize their
> efforts. For reference sake, you can find the previous attempts at:
>
> https://sourceware.org/ml/gdb-patches/2006-09/msg00192.html
>
> https://sourceware.org/ml/gdb-patches/2014-02/msg00248.html
>
> https://sourceware.org/ml/gdb-patches/2016-02/msg00226.html
Ping.
> The basic idea behind the patch is to start using the new
> 'getaddrinfo'/'getnameinfo' calls, which are responsible for
> translating names and addresses in a protocol-independent way. This
> means that if we ever have an IPv8, we won't need to change the code
> again.
>
> The function 'getaddrinfo' returns a linked list of possible addresses
> to connect to, so I modified ser-tcp.c:net_open's code to loop through
> the linked list and try all the addresses until it finds a valid one.
> The same rationale was used for gdbserver, but without the "retry"
> mechanism that GDB has.
>
> I also implemented some hostname parsing functions which are used to
> help GDB and gdbserver to parse hostname strings provided by the user.
> These new functions are living inside common/netstuff.[ch]. I've had
> to do that since IPv6 introduces a new URL scheme, which defines that
> square brackets can be used to enclose the host part and differentiate
> it from the port (e.g., "[::1]:1234" means "host ::1, port 1234"). I
> spent some time thinking about a reasonable way to interpret what the
> user wants, and I came up with the following:
>
> - If the user has provided a prefix that doesn't specify the protocol
> version (i.e., "tcp:" or "udp:"), or if the user has not provided
> any prefix, don't make any assumptions (i.e., assume AF_UNSPEC when
> dealing with 'getaddrinfo') *unless* the host starts with "[" (in
> which case, assume it's an IPv6 host).
>
> - If the user has provided a prefix that does specify the protocol
> version (i.e., "tcp4:", "tcp6:", "udp4:" or "udp6:"), then respect
> that.
>
> This method doesn't follow strictly what RFC 2732 proposes (that
> literal IPv6 addresses should be provided enclosed in "[" and "]")
> because IPv6 addresses still can be provided without square brackets
> in our case, but since we have prefixes to specify protocol versions I
> think this is not an issue.
>
> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
> parameter, which instructs GDB and gdbserver to use IPv6 for
> connections. This way, if you want to run IPv6 tests, you do:
>
> $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
>
> This required a few changes on the gdbserver-base.exp,
> native-gdbserver.exp and native-extended-gdbserver.exp boards, and
> also a minimal adjustment on gdb.server/run-without-local-binary.exp.
>
> This patch has been regression-tested on BuildBot and locally, and
> also built using a x86_64-w64-mingw32 GCC, and no problems were found.
>
> gdb/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * Makefile.in (COMMON_SFILES): Add 'common/netstuff.c'.
> (HFILES_NO_SRCDIR): Add 'common/netstuff.h'.
> * NEWS (Changes since GDB 8.1): Mention IPv6 support.
> * common/netstuff.c: New file.
> * common/netstuff.h: New file.
> * ser-tcp.c: Include 'netstuff.h' and 'wspiapi.h'.
> (net_open): Handle IPv6-style hostnames; implement support for
> IPv6 connections.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * Makefile.in (SFILES): Add '$(srcdir)/common/netstuff.c'.
> (OBS): Add 'common/netstuff.o'.
> * gdbreplay.c: Include 'wspiapi.h'.
> (remote_open): Implement support for IPv6
> connections.
> * remote-utils.c: Include 'netstuff.h', 'filestuff.h'
> and 'wspiapi.h'.
> (handle_accept_event): Accept connections from IPv6 sources.
> (remote_prepare): Handle IPv6-style hostnames; implement
> support for IPv6 connections.
> (remote_open): Implement support for printing connections from
> IPv6 sources.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * README (Testsuite Parameters): Mention new 'GDB_TEST_IPV6'
> parameter.
> * boards/gdbserver-base.exp (get_comm_port_localhost_ipv6):
> New procedure.
> * boards/native-extended-gdbserver.exp: Detect 'GDB_TEST_IPV6'
> and change board info accordingly.
> * boards/native-gdbserver.exp: Likewise.
> * gdb.server/run-without-local-binary.exp: Improve regexp used
> for detecting when a remote debugging connection succeeds.
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Paul Fertser <fercerpav@gmail.com>
> Tsutomu Seki <sekiriki@gmail.com>
>
> * gdb.texinfo (Remote Connection Commands): Add explanation
> about new IPv6 support. Add new connection prefixes.
> ---
> gdb/Makefile.in | 2 +
> gdb/NEWS | 4 +
> gdb/common/netstuff.c | 136 +++++++++++++
> gdb/common/netstuff.h | 52 +++++
> gdb/doc/gdb.texinfo | 48 ++++-
> gdb/gdbserver/Makefile.in | 2 +
> gdb/gdbserver/gdbreplay.c | 181 +++++++++++++----
> gdb/gdbserver/remote-utils.c | 119 +++++++----
> gdb/ser-tcp.c | 217 ++++++++++-----------
> gdb/testsuite/README | 7 +
> gdb/testsuite/boards/gdbserver-base.exp | 5 +
> gdb/testsuite/boards/native-extended-gdbserver.exp | 7 +-
> gdb/testsuite/boards/native-gdbserver.exp | 7 +-
> .../gdb.server/run-without-local-binary.exp | 2 +-
> 14 files changed, 602 insertions(+), 187 deletions(-)
> create mode 100644 gdb/common/netstuff.c
> create mode 100644 gdb/common/netstuff.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index df6ebab851..06ce12a4ee 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -962,6 +962,7 @@ COMMON_SFILES = \
> common/job-control.c \
> common/gdb_tilde_expand.c \
> common/gdb_vecs.c \
> + common/netstuff.c \
> common/new-op.c \
> common/pathstuff.c \
> common/print-utils.c \
> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
> common/gdb_vecs.h \
> common/gdb_wait.h \
> common/common-inferior.h \
> + common/netstuff.h \
> common/host-defs.h \
> common/pathstuff.h \
> common/print-utils.h \
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cef558039e..1f95ced912 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>
> *** Changes since GDB 8.1
>
> +* GDB and GDBserver now support IPv6 connections. IPv6 hostnames
> + can be passed using the '[ADDRESS]:PORT' notation, or the regular
> + 'ADDRESS:PORT' method.
> +
> * The commands 'info variables/functions/types' now show the source line
> numbers of symbol definitions when available.
>
> diff --git a/gdb/common/netstuff.c b/gdb/common/netstuff.c
> new file mode 100644
> index 0000000000..cdf4b611db
> --- /dev/null
> +++ b/gdb/common/netstuff.c
> @@ -0,0 +1,136 @@
> +/* Operations on network stuff.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include "common-defs.h"
> +#include "netstuff.h"
> +
> +#ifdef USE_WIN32API
> +#include <winsock2.h>
> +#include <wspiapi.h>
> +#else
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +#include <netdb.h>
> +#include <sys/socket.h>
> +#include <netinet/tcp.h>
> +#endif
> +
> +/* See common/netstuff.h. */
> +
> +scoped_free_addrinfo::scoped_free_addrinfo (struct addrinfo *a)
> +{
> + m_res = a;
> +}
> +/* See common/netstuff.h. */
> +
> +scoped_free_addrinfo::~scoped_free_addrinfo ()
> +{
> + freeaddrinfo (m_res);
> +}
> +
> +/* See common/netstuff.h. */
> +
> +void
> +parse_hostname_without_prefix (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint)
> +{
> + std::string strname (hostname);
> +
> + if (hint->ai_family != AF_INET && strname[0] == '[')
> + {
> + /* IPv6 addresses can be written as '[ADDR]:PORT', and we
> + support this notation. */
> + size_t close_bracket_pos = strname.find_first_of (']');
> +
> + if (close_bracket_pos == std::string::npos)
> + error (_("Missing close bracket in hostname '%s'"),
> + strname.c_str ());
> +
> + /* Erase both '[' and ']'. */
> + strname.erase (0, 1);
> + strname.erase (close_bracket_pos - 1, 1);
> +
> + hint->ai_family = AF_INET6;
> + }
> +
> + /* The length of the hostname part. */
> + size_t host_len;
> +
> + size_t last_colon_pos = strname.find_last_of (':');
> +
> + if (last_colon_pos != std::string::npos)
> + {
> + /* The user has provided a port. */
> + host_len = last_colon_pos;
> + port_str = strname.substr (last_colon_pos + 1);
> + }
> + else
> + host_len = strname.size ();
> +
> + host_str = strname.substr (0, host_len);
> +
> + /* Default hostname is localhost. */
> + if (host_str.empty ())
> + host_str = "localhost";
> +}
> +
> +/* See common/netstuff.h. */
> +
> +void
> +parse_hostname (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint)
> +{
> + /* Struct to hold the association between valid prefixes, their
> + family and socktype. */
> + struct host_prefix
> + {
> + /* The prefix. */
> + const char *prefix;
> +
> + /* The 'ai_family'. */
> + int family;
> +
> + /* The 'ai_socktype'. */
> + int socktype;
> + };
> + static const struct host_prefix prefixes[] =
> + {
> + { "udp:", AF_UNSPEC, SOCK_DGRAM },
> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
> + { "udp4:", AF_INET, SOCK_DGRAM },
> + { "tcp4:", AF_INET, SOCK_STREAM },
> + { "udp6:", AF_INET6, SOCK_DGRAM },
> + { "tcp6:", AF_INET6, SOCK_STREAM },
> + { NULL, 0, 0 },
> + };
> +
> + for (const struct host_prefix *prefix = prefixes;
> + prefix->prefix != NULL;
> + ++prefix)
> + if (startswith (hostname, prefix->prefix))
> + {
> + hostname += strlen (prefix->prefix);
> + hint->ai_family = prefix->family;
> + hint->ai_socktype = prefix->socktype;
> + hint->ai_protocol
> + = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
> + break;
> + }
> +
> + parse_hostname_without_prefix (hostname, host_str, port_str, hint);
> +}
> diff --git a/gdb/common/netstuff.h b/gdb/common/netstuff.h
> new file mode 100644
> index 0000000000..1ac2433f11
> --- /dev/null
> +++ b/gdb/common/netstuff.h
> @@ -0,0 +1,52 @@
> +/* Operations on network stuff.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef NETSTUFF_H
> +#define NETSTUFF_H
> +
> +#include <string>
> +
> +/* Helper class to guarantee that we always call 'freeaddrinfo'. */
> +
> +class scoped_free_addrinfo
> +{
> +public:
> + scoped_free_addrinfo (struct addrinfo *a);
> +
> + ~scoped_free_addrinfo ();
> +
> + DISABLE_COPY_AND_ASSIGN (scoped_free_addrinfo);
> +
> +private:
> + struct addrinfo *m_res;
> +};
> +
> +/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill
> + in HOST_STR, PORT_STR and HINT accordingly. */
> +extern void parse_hostname_without_prefix (const char *hostname,
> + std::string &host_str,
> + std::string &port_str,
> + struct addrinfo *hint);
> +
> +/* Parse HOSTNAME (which is a string in the form of
> + "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
> + HINT accordingly. */
> +extern void parse_hostname (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint);
> +
> +#endif /* ! NETSTUFF_H */
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 28f083f96e..7994204140 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -20496,16 +20496,27 @@ If you're using a serial line, you may want to give @value{GDBN} the
> @code{target} command.
>
> @item target remote @code{@var{host}:@var{port}}
> +@itemx target remote @code{@var{@r{[}host@r{]}}:@var{port}}
> @itemx target remote @code{tcp:@var{host}:@var{port}}
> +@itemx target remote @code{tcp:@var{@r{[}host@r{]}}:@var{port}}
> +@itemx target remote @code{tcp4:@var{host}:@var{port}}
> +@itemx target remote @code{tcp6:@var{host}:@var{port}}
> +@itemx target remote @code{tcp6:@var{@r{[}host@r{]}}:@var{port}}
> @itemx target extended-remote @code{@var{host}:@var{port}}
> +@itemx target extended-remote @code{@var{@r{[}host@r{]}}:@var{port}}
> @itemx target extended-remote @code{tcp:@var{host}:@var{port}}
> +@itemx target extended-remote @code{tcp:@var{@r{[}host@r{]}}:@var{port}}
> +@itemx target extended-remote @code{tcp4:@var{host}:@var{port}}
> +@itemx target extended-remote @code{tcp6:@var{host}:@var{port}}
> +@itemx target extended-remote @code{tcp6:@var{@r{[}host@r{]}}:@var{port}}
> @cindex @acronym{TCP} port, @code{target remote}
> Debug using a @acronym{TCP} connection to @var{port} on @var{host}.
> -The @var{host} may be either a host name or a numeric @acronym{IP}
> -address; @var{port} must be a decimal number. The @var{host} could be
> -the target machine itself, if it is directly connected to the net, or
> -it might be a terminal server which in turn has a serial line to the
> -target.
> +The @var{host} may be either a host name, a numeric @acronym{IPv4}
> +address, or a numeric @acronym{IPv6} address (with or without the
> +square brackets to separate the address from the port); @var{port}
> +must be a decimal number. The @var{host} could be the target machine
> +itself, if it is directly connected to the net, or it might be a
> +terminal server which in turn has a serial line to the target.
>
> For example, to connect to port 2828 on a terminal server named
> @code{manyfarms}:
> @@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
> target remote manyfarms:2828
> @end smallexample
>
> +To connect to port 2828 on a terminal server whose address is
> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
> +
> +@smallexample
> +target remote [2001::f8ff::67cf]:2828
> +@end smallexample
> +
> +Or explicitly specify the @acronym{IPv6} protocol:
> +
> +@smallexample
> +target remote tcp6:2001::f8ff::67cf:2828
> +@end smallexample
> +
> +This last example may be confusing to the reader, because there is no
> +visible separation between the hostname and the port number.
> +Therefore, we recommend the user to provide @acronym{IPv6} addresses
> +using square brackets for clarity.
> +
> If your remote target is actually running on the same machine as your
> debugger session (e.g.@: a simulator for your target running on the
> same host), you can omit the hostname. For example, to connect to
> @@ -20527,7 +20556,16 @@ target remote :1234
> Note that the colon is still required here.
>
> @item target remote @code{udp:@var{host}:@var{port}}
> +@itemx target remote @code{udp:@var{host}:@var{port}}
> +@itemx target remote @code{udp:@var{@r{[}host@r{]}}:@var{port}}
> +@itemx target remote @code{udp4:@var{host}:@var{port}}
> +@itemx target remote @code{udp6:@var{@r{[}host@r{]}}:@var{port}}
> +@itemx target extended-remote @code{udp:@var{host}:@var{port}}
> @itemx target extended-remote @code{udp:@var{host}:@var{port}}
> +@itemx target extended-remote @code{udp:@var{@r{[}host@r{]}}:@var{port}}
> +@itemx target extended-remote @code{udp4:@var{host}:@var{port}}
> +@itemx target extended-remote @code{udp6:@var{host}:@var{port}}
> +@itemx target extended-remote @code{udp6:@var{@r{[}host@r{]}}:@var{port}}
> @cindex @acronym{UDP} port, @code{target remote}
> Debug using @acronym{UDP} packets to @var{port} on @var{host}. For example, to
> connect to @acronym{UDP} port 2828 on a terminal server named @code{manyfarms}:
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index 675faa4364..d65346a357 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -211,6 +211,7 @@ SFILES = \
> $(srcdir)/common/job-control.c \
> $(srcdir)/common/gdb_tilde_expand.c \
> $(srcdir)/common/gdb_vecs.c \
> + $(srcdir)/common/netstuff.c \
> $(srcdir)/common/new-op.c \
> $(srcdir)/common/pathstuff.c \
> $(srcdir)/common/print-utils.c \
> @@ -253,6 +254,7 @@ OBS = \
> common/format.o \
> common/gdb_tilde_expand.o \
> common/gdb_vecs.o \
> + common/netstuff.o \
> common/new-op.o \
> common/pathstuff.o \
> common/print-utils.o \
> diff --git a/gdb/gdbserver/gdbreplay.c b/gdb/gdbserver/gdbreplay.c
> index c1a639069a..01b70d49f4 100644
> --- a/gdb/gdbserver/gdbreplay.c
> +++ b/gdb/gdbserver/gdbreplay.c
> @@ -53,6 +53,7 @@
>
> #if USE_WIN32API
> #include <winsock2.h>
> +#include <wspiapi.h>
> #endif
>
> #ifndef HAVE_SOCKLEN_T
> @@ -175,56 +176,159 @@ remote_close (void)
> static void
> remote_open (char *name)
> {
> - if (!strchr (name, ':'))
> + char *last_colon = strrchr (name, ':');
> +
> + if (last_colon == NULL)
> {
> fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
> fflush (stderr);
> exit (1);
> }
> - else
> - {
> +
> #ifdef USE_WIN32API
> - static int winsock_initialized;
> + static int winsock_initialized;
> #endif
> - char *port_str;
> - int port;
> - struct sockaddr_in sockaddr;
> - socklen_t tmp;
> - int tmp_desc;
> + char *port_str;
> + int tmp;
> + int tmp_desc;
> + struct addrinfo hint;
> + struct addrinfo *ainfo;
> + char *orig_name = strdup (name);
> +
> + struct prefix
> + {
> + /* The prefix to be parsed. */
> + const char *str;
> +
> + /* The address family. */
> + int ai_family;
> +
> + /* The socktype. */
> + int ai_socktype;
> + };
> + static const struct prefix prefixes[]
> + = { { "udp:", AF_UNSPEC, SOCK_DGRAM },
> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
> + { "udp4:", AF_INET, SOCK_DGRAM },
> + { "tcp4:", AF_INET, SOCK_STREAM },
> + { "udp6:", AF_INET6, SOCK_DGRAM },
> + { "tcp6:", AF_INET6, SOCK_STREAM },
> + { NULL, 0, 0 } };
> +
> + memset (&hint, 0, sizeof (hint));
> + /* Assume no prefix will be passed, therefore we should use
> + AF_UNSPEC. */
> + hint.ai_family = AF_UNSPEC;
> + hint.ai_socktype = SOCK_STREAM;
> + hint.ai_protocol = IPPROTO_TCP;
> +
> + for (const struct prefix *p = prefixes; p->str != NULL; ++p)
> + if (strncmp (name, p->str, strlen (p->str)) == 0)
> + {
> + name += strlen (p->str);
> + hint.ai_family = p->ai_family;
> + hint.ai_socktype = p->ai_socktype;
> + hint.ai_protocol
> + = p->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
> + break;
> + }
>
> - port_str = strchr (name, ':');
> + if (hint.ai_family != AF_INET && *name == '[')
> + {
> + ++name;
> + port_str = strchr (name, ']');
> + if (port_str == NULL)
> + {
> + fprintf (stderr, "Missing closing bracket on hostname: %s\n",
> + orig_name);
> + exit (1);
> + }
> + /* Skip closing bracket. */
> + *port_str = '\0';
> + ++port_str;
> + if (*port_str != ':')
> + {
> + fprintf (stderr, "Missing port number on hostname: %s\n",
> + orig_name);
> + exit (1);
> + }
> + hint.ai_family = AF_INET6;
> + }
> + else
> + port_str = last_colon;
>
> - port = atoi (port_str + 1);
> + /* Skip the colon. */
> + *port_str = '\0';
> + ++port_str;
>
> #ifdef USE_WIN32API
> - if (!winsock_initialized)
> - {
> - WSADATA wsad;
> + if (!winsock_initialized)
> + {
> + WSADATA wsad;
>
> - WSAStartup (MAKEWORD (1, 0), &wsad);
> - winsock_initialized = 1;
> - }
> + WSAStartup (MAKEWORD (1, 0), &wsad);
> + winsock_initialized = 1;
> + }
> #endif
>
> - tmp_desc = socket (PF_INET, SOCK_STREAM, 0);
> - if (tmp_desc == -1)
> - perror_with_name ("Can't open socket");
> + int r = getaddrinfo (name, port_str, &hint, &ainfo);
>
> - /* Allow rapid reuse of this port. */
> - tmp = 1;
> - setsockopt (tmp_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> - sizeof (tmp));
> + if (r != 0)
> + {
> + fprintf (stderr, "%s:%s: cannot resolve name: %s\n",
> + name, port_str, gai_strerror (r));
> + fflush (stderr);
> + exit (1);
> + }
>
> - sockaddr.sin_family = PF_INET;
> - sockaddr.sin_port = htons (port);
> - sockaddr.sin_addr.s_addr = INADDR_ANY;
> + struct addrinfo *p;
>
> - if (bind (tmp_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
> - || listen (tmp_desc, 1))
> - perror_with_name ("Can't bind address");
> + for (p = ainfo; p != NULL; p = p->ai_next)
> + {
> + tmp_desc = socket (p->ai_family, p->ai_socktype, p->ai_protocol);
> +
> + if (tmp_desc >= 0)
> + break;
> + }
> +
> + if (p == NULL)
> + perror_with_name ("Cannot open socket");
> +
> + /* Allow rapid reuse of this port. */
> + tmp = 1;
> + setsockopt (tmp_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> + sizeof (tmp));
> +
> + switch (p->ai_family)
> + {
> + case AF_INET:
> + ((struct sockaddr_in *) p->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> + break;
> + case AF_INET6:
> + ((struct sockaddr_in6 *) p->ai_addr)->sin6_addr = in6addr_any;
> + break;
> + default:
> + fprintf (stderr, "Invalid 'ai_family' %d\n", p->ai_family);
> + exit (1);
> + }
> +
> + if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
> + perror_with_name ("Can't bind address");
> +
> + if (p->ai_socktype == SOCK_DGRAM)
> + remote_desc = tmp_desc;
> + else
> + {
> + struct sockaddr_storage sockaddr;
> + socklen_t sockaddrsize = sizeof (sockaddr);
> + char orig_host[64], orig_port[16];
> +
> + if (listen (tmp_desc, 1) != 0)
> + perror_with_name ("Can't listen on socket");
> +
> + remote_desc = accept (tmp_desc, (struct sockaddr *) &sockaddr,
> + &sockaddrsize);
>
> - tmp = sizeof (sockaddr);
> - remote_desc = accept (tmp_desc, (struct sockaddr *) &sockaddr, &tmp);
> if (remote_desc == -1)
> perror_with_name ("Accept failed");
>
> @@ -239,6 +343,16 @@ remote_open (char *name)
> setsockopt (remote_desc, IPPROTO_TCP, TCP_NODELAY,
> (char *) &tmp, sizeof (tmp));
>
> + if (getnameinfo ((struct sockaddr *) &sockaddr, sockaddrsize,
> + orig_host, sizeof (orig_host),
> + orig_port, sizeof (orig_port),
> + NI_NUMERICHOST | NI_NUMERICSERV) == 0)
> + {
> + fprintf (stderr, "Remote debugging from host %s, port %s\n",
> + orig_host, orig_port);
> + fflush (stderr);
> + }
> +
> #ifndef USE_WIN32API
> close (tmp_desc); /* No longer need this */
>
> @@ -254,8 +368,9 @@ remote_open (char *name)
> fcntl (remote_desc, F_SETFL, FASYNC);
> #endif
>
> - fprintf (stderr, "Replay logfile using %s\n", name);
> + fprintf (stderr, "Replay logfile using %s\n", orig_name);
> fflush (stderr);
> + free (orig_name);
> }
>
> static int
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 3b5a459ae4..c8b5dcdbba 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -26,6 +26,8 @@
> #include "dll.h"
> #include "rsp-low.h"
> #include "gdbthread.h"
> +#include "netstuff.h"
> +#include "filestuff.h"
> #include <ctype.h>
> #if HAVE_SYS_IOCTL_H
> #include <sys/ioctl.h>
> @@ -63,6 +65,7 @@
>
> #if USE_WIN32API
> #include <winsock2.h>
> +#include <wspiapi.h>
> #endif
>
> #if __QNX__
> @@ -156,19 +159,18 @@ enable_async_notification (int fd)
> static int
> handle_accept_event (int err, gdb_client_data client_data)
> {
> - struct sockaddr_in sockaddr;
> - socklen_t tmp;
> + struct sockaddr_storage sockaddr;
> + socklen_t len = sizeof (sockaddr);
>
> if (debug_threads)
> debug_printf ("handling possible accept event\n");
>
> - tmp = sizeof (sockaddr);
> - remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
> + remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
> if (remote_desc == -1)
> perror_with_name ("Accept failed");
>
> /* Enable TCP keep alive process. */
> - tmp = 1;
> + socklen_t tmp = 1;
> setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
> (char *) &tmp, sizeof (tmp));
>
> @@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
> delete_file_handler (listen_desc);
>
> /* Convert IP address to string. */
> - fprintf (stderr, "Remote debugging from host %s\n",
> - inet_ntoa (sockaddr.sin_addr));
> + char orig_host[64], orig_port[16];
> +
> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> + orig_host, sizeof (orig_host),
> + orig_port, sizeof (orig_port),
> + NI_NUMERICHOST | NI_NUMERICSERV);
> +
> + if (r != 0)
> + fprintf (stderr, _("Could not obtain remote address: %s\n"),
> + gai_strerror (r));
> + else
> + fprintf (stderr, "Remote debugging from host %s, port %s\n", orig_host,
> + orig_port);
>
> enable_async_notification (remote_desc);
>
> @@ -222,14 +235,10 @@ handle_accept_event (int err, gdb_client_data client_data)
> void
> remote_prepare (const char *name)
> {
> - const char *port_str;
> #ifdef USE_WIN32API
> static int winsock_initialized;
> #endif
> - int port;
> - struct sockaddr_in sockaddr;
> socklen_t tmp;
> - char *port_end;
>
> remote_is_stdio = 0;
> if (strcmp (name, STDIO_CONNECTION_NAME) == 0)
> @@ -242,17 +251,25 @@ remote_prepare (const char *name)
> return;
> }
>
> - port_str = strchr (name, ':');
> - if (port_str == NULL)
> + struct addrinfo hint;
> + struct addrinfo *ainfo;
> + std::string host_str, port_str;
> +
> + memset (&hint, 0, sizeof (hint));
> + /* Assume no prefix will be passed, therefore we should use
> + AF_UNSPEC. */
> + hint.ai_family = AF_UNSPEC;
> + hint.ai_socktype = SOCK_STREAM;
> + hint.ai_protocol = IPPROTO_TCP;
> +
> + parse_hostname_without_prefix (name, host_str, port_str, &hint);
> +
> + if (port_str.empty ())
> {
> transport_is_reliable = 0;
> return;
> }
>
> - port = strtoul (port_str + 1, &port_end, 10);
> - if (port_str[1] == '\0' || *port_end != '\0')
> - error ("Bad port argument: %s", name);
> -
> #ifdef USE_WIN32API
> if (!winsock_initialized)
> {
> @@ -263,8 +280,25 @@ remote_prepare (const char *name)
> }
> #endif
>
> - listen_desc = socket (PF_INET, SOCK_STREAM, IPPROTO_TCP);
> - if (listen_desc == -1)
> + int r = getaddrinfo (host_str.c_str (), port_str.c_str (), &hint, &ainfo);
> +
> + if (r != 0)
> + error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
> +
> + scoped_free_addrinfo freeaddrinfo (ainfo);
> +
> + struct addrinfo *iter;
> +
> + for (iter = ainfo; iter != NULL; iter = iter->ai_next)
> + {
> + listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
> + iter->ai_protocol);
> +
> + if (listen_desc >= 0)
> + break;
> + }
> +
> + if (iter == NULL)
> perror_with_name ("Can't open socket");
>
> /* Allow rapid reuse of this port. */
> @@ -272,14 +306,25 @@ remote_prepare (const char *name)
> setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> sizeof (tmp));
>
> - sockaddr.sin_family = PF_INET;
> - sockaddr.sin_port = htons (port);
> - sockaddr.sin_addr.s_addr = INADDR_ANY;
> + switch (iter->ai_family)
> + {
> + case AF_INET:
> + ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> + break;
> + case AF_INET6:
> + ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
> + break;
> + default:
> + internal_error (__FILE__, __LINE__,
> + _("Invalid 'ai_family' %d\n"), iter->ai_family);
> + }
>
> - if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
> - || listen (listen_desc, 1))
> + if (bind (listen_desc, iter->ai_addr, iter->ai_addrlen) != 0)
> perror_with_name ("Can't bind address");
>
> + if (listen (listen_desc, 1) != 0)
> + perror_with_name ("Can't listen on socket");
> +
> transport_is_reliable = 1;
> }
>
> @@ -354,18 +399,24 @@ remote_open (const char *name)
> #endif /* USE_WIN32API */
> else
> {
> - int port;
> - socklen_t len;
> - struct sockaddr_in sockaddr;
> -
> - len = sizeof (sockaddr);
> - if (getsockname (listen_desc,
> - (struct sockaddr *) &sockaddr, &len) < 0
> - || len < sizeof (sockaddr))
> + char listen_port[16];
> + struct sockaddr_storage sockaddr;
> + socklen_t len = sizeof (sockaddr);
> +
> + if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
> perror_with_name ("Can't determine port");
> - port = ntohs (sockaddr.sin_port);
>
> - fprintf (stderr, "Listening on port %d\n", port);
> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> + NULL, 0,
> + listen_port, sizeof (listen_port),
> + NI_NUMERICSERV);
> +
> + if (r != 0)
> + fprintf (stderr, _("Can't obtain port where we are listening: %s"),
> + gai_strerror (r));
> + else
> + fprintf (stderr, "Listening on port %s\n", listen_port);
> +
> fflush (stderr);
>
> /* Register the event loop handler. */
> diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
> index 23ef3b04b8..3d9fbd866f 100644
> --- a/gdb/ser-tcp.c
> +++ b/gdb/ser-tcp.c
> @@ -25,6 +25,7 @@
> #include "cli/cli-decode.h"
> #include "cli/cli-setshow.h"
> #include "filestuff.h"
> +#include "netstuff.h"
>
> #include <sys/types.h>
>
> @@ -39,6 +40,7 @@
>
> #ifdef USE_WIN32API
> #include <winsock2.h>
> +#include <wspiapi.h>
> #ifndef ETIMEDOUT
> #define ETIMEDOUT WSAETIMEDOUT
> #endif
> @@ -158,166 +160,157 @@ wait_for_connect (struct serial *scb, unsigned int *polls)
> int
> net_open (struct serial *scb, const char *name)
> {
> - char hostname[100];
> - const char *port_str;
> - int n, port, tmp;
> - int use_udp;
> - struct hostent *hostent;
> - struct sockaddr_in sockaddr;
> + int n;
> + bool use_udp;
> #ifdef USE_WIN32API
> u_long ioarg;
> #else
> int ioarg;
> #endif
> unsigned int polls = 0;
> + struct addrinfo hint;
> + struct addrinfo *ainfo;
> + std::string host_str, port_str;
>
> - use_udp = 0;
> - if (startswith (name, "udp:"))
> - {
> - use_udp = 1;
> - name = name + 4;
> - }
> - else if (startswith (name, "tcp:"))
> - name = name + 4;
> -
> - port_str = strchr (name, ':');
> + memset (&hint, 0, sizeof (hint));
> + /* Assume no prefix will be passed, therefore we should use
> + AF_UNSPEC. */
> + hint.ai_family = AF_INET;
> + hint.ai_socktype = SOCK_STREAM;
> + hint.ai_protocol = IPPROTO_TCP;
>
> - if (!port_str)
> - error (_("net_open: No colon in host name!")); /* Shouldn't ever
> - happen. */
> + parse_hostname (name, host_str, port_str, &hint);
>
> - tmp = std::min (port_str - name, (ptrdiff_t) sizeof hostname - 1);
> - strncpy (hostname, name, tmp); /* Don't want colon. */
> - hostname[tmp] = '\000'; /* Tie off host name. */
> - port = atoi (port_str + 1);
> + if (port_str.empty ())
> + error (_("Missing port on hostname '%s'"), name);
>
> - /* Default hostname is localhost. */
> - if (!hostname[0])
> - strcpy (hostname, "localhost");
> + int r = getaddrinfo (host_str.c_str (), port_str.c_str (), &hint, &ainfo);
>
> - hostent = gethostbyname (hostname);
> - if (!hostent)
> + if (r != 0)
> {
> - fprintf_unfiltered (gdb_stderr, "%s: unknown host\n", hostname);
> + fprintf_unfiltered (gdb_stderr, _("%s: cannot resolve name: %s\n"),
> + name, gai_strerror (r));
> errno = ENOENT;
> return -1;
> }
>
> - sockaddr.sin_family = PF_INET;
> - sockaddr.sin_port = htons (port);
> - memcpy (&sockaddr.sin_addr.s_addr, hostent->h_addr,
> - sizeof (struct in_addr));
> + scoped_free_addrinfo free_ainfo (ainfo);
>
> - retry:
> + struct addrinfo *cur_ainfo;
>
> - if (use_udp)
> - scb->fd = gdb_socket_cloexec (PF_INET, SOCK_DGRAM, 0);
> - else
> - scb->fd = gdb_socket_cloexec (PF_INET, SOCK_STREAM, 0);
> + for (cur_ainfo = ainfo; cur_ainfo != NULL; cur_ainfo = cur_ainfo->ai_next)
> + {
> +retry:
> + scb->fd = gdb_socket_cloexec (cur_ainfo->ai_family,
> + cur_ainfo->ai_socktype,
> + cur_ainfo->ai_protocol);
>
> - if (scb->fd == -1)
> - return -1;
> -
> - /* Set socket nonblocking. */
> - ioarg = 1;
> - ioctl (scb->fd, FIONBIO, &ioarg);
> + if (scb->fd < 0)
> + continue;
>
> - /* Use Non-blocking connect. connect() will return 0 if connected
> - already. */
> - n = connect (scb->fd, (struct sockaddr *) &sockaddr, sizeof (sockaddr));
> + /* Set socket nonblocking. */
> + ioarg = 1;
> + ioctl (scb->fd, FIONBIO, &ioarg);
>
> - if (n < 0)
> - {
> + /* Use Non-blocking connect. connect() will return 0 if connected
> + already. */
> + n = connect (scb->fd, cur_ainfo->ai_addr, cur_ainfo->ai_addrlen);
> +
> + if (n < 0)
> + {
> #ifdef USE_WIN32API
> - int err = WSAGetLastError();
> + int err = WSAGetLastError();
> #else
> - int err = errno;
> + int err = errno;
> #endif
>
> - /* Maybe we're waiting for the remote target to become ready to
> - accept connections. */
> - if (tcp_auto_retry
> + /* Maybe we're waiting for the remote target to become ready to
> + accept connections. */
> + if (tcp_auto_retry
> #ifdef USE_WIN32API
> - && err == WSAECONNREFUSED
> + && err == WSAECONNREFUSED
> #else
> - && err == ECONNREFUSED
> + && err == ECONNREFUSED
> #endif
> - && wait_for_connect (NULL, &polls) >= 0)
> - {
> - close (scb->fd);
> - goto retry;
> - }
> + && wait_for_connect (NULL, &polls) >= 0)
> + {
> + close (scb->fd);
> + goto retry;
> + }
>
> - if (
> + if (
> #ifdef USE_WIN32API
> - /* Under Windows, calling "connect" with a non-blocking socket
> - results in WSAEWOULDBLOCK, not WSAEINPROGRESS. */
> - err != WSAEWOULDBLOCK
> + /* Under Windows, calling "connect" with a non-blocking socket
> + results in WSAEWOULDBLOCK, not WSAEINPROGRESS. */
> + err != WSAEWOULDBLOCK
> #else
> - err != EINPROGRESS
> + err != EINPROGRESS
> #endif
> - )
> - {
> - errno = err;
> - net_close (scb);
> - return -1;
> + )
> + {
> + errno = err;
> + continue;
> + }
> +
> + /* Looks like we need to wait for the connect. */
> + do
> + {
> + n = wait_for_connect (scb, &polls);
> + }
> + while (n == 0);
> + if (n < 0)
> + continue;
> }
>
> - /* Looks like we need to wait for the connect. */
> - do
> - {
> - n = wait_for_connect (scb, &polls);
> - }
> - while (n == 0);
> - if (n < 0)
> - {
> - net_close (scb);
> - return -1;
> - }
> - }
> -
> - /* Got something. Is it an error? */
> - {
> - int res, err;
> - socklen_t len;
> -
> - len = sizeof (err);
> - /* On Windows, the fourth parameter to getsockopt is a "char *";
> - on UNIX systems it is generally "void *". The cast to "char *"
> - is OK everywhere, since in C++ any data pointer type can be
> - implicitly converted to "void *". */
> - res = getsockopt (scb->fd, SOL_SOCKET, SO_ERROR, (char *) &err, &len);
> - if (res < 0 || err)
> + /* Got something. Is it an error? */
> {
> - /* Maybe the target still isn't ready to accept the connection. */
> - if (tcp_auto_retry
> + int res, err;
> + socklen_t len = sizeof (err);
> +
> + /* On Windows, the fourth parameter to getsockopt is a "char *";
> + on UNIX systems it is generally "void *". The cast to "char *"
> + is OK everywhere, since in C++ any data pointer type can be
> + implicitly converted to "void *". */
> + res = getsockopt (scb->fd, SOL_SOCKET, SO_ERROR, (char *) &err, &len);
> + if (res < 0 || err)
> + {
> + /* Maybe the target still isn't ready to accept the connection. */
> + if (tcp_auto_retry
> #ifdef USE_WIN32API
> - && err == WSAECONNREFUSED
> + && err == WSAECONNREFUSED
> #else
> - && err == ECONNREFUSED
> + && err == ECONNREFUSED
> #endif
> - && wait_for_connect (NULL, &polls) >= 0)
> - {
> - close (scb->fd);
> - goto retry;
> + && wait_for_connect (NULL, &polls) >= 0)
> + {
> + close (scb->fd);
> + goto retry;
> + }
> + if (err)
> + errno = err;
> + continue;
> }
> - if (err)
> - errno = err;
> - net_close (scb);
> - return -1;
> }
> - }
> + break;
> + }
> +
> + if (cur_ainfo == NULL)
> + {
> + net_close (scb);
> + return -1;
> + }
>
> /* Turn off nonblocking. */
> ioarg = 0;
> ioctl (scb->fd, FIONBIO, &ioarg);
>
> - if (use_udp == 0)
> + if (cur_ainfo->ai_socktype == IPPROTO_TCP)
> {
> /* Disable Nagle algorithm. Needed in some cases. */
> - tmp = 1;
> + int tmp = 1;
> +
> setsockopt (scb->fd, IPPROTO_TCP, TCP_NODELAY,
> - (char *)&tmp, sizeof (tmp));
> + (char *) &tmp, sizeof (tmp));
> }
>
> #ifdef SIGPIPE
> diff --git a/gdb/testsuite/README b/gdb/testsuite/README
> index 4475ac21a9..37f676d252 100644
> --- a/gdb/testsuite/README
> +++ b/gdb/testsuite/README
> @@ -259,6 +259,13 @@ This make (not runtest) variable is used to specify whether the
> testsuite preloads the read1.so library into expect. Any non-empty
> value means true. See "Race detection" below.
>
> +GDB_TEST_IPV6
> +
> +This variable makes the tests related to GDBserver to run with IPv6
> +local addresses, instead of IPv4. This is useful to test the IPv6
> +support, and only makes sense for the native-gdbserver and the
> +native-extended-gdbserver boards.
> +
> Race detection
> **************
>
> diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
> index 52ad698b3f..f738c90e8e 100644
> --- a/gdb/testsuite/boards/gdbserver-base.exp
> +++ b/gdb/testsuite/boards/gdbserver-base.exp
> @@ -32,3 +32,8 @@ set_board_info gdb,nofileio 1
> set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
>
> set GDBFLAGS "${GDBFLAGS} -ex \"set auto-connect-native-target off\""
> +
> +# Helper function that returns a local IPv6 address to connect to.
> +proc get_comm_port_localhost_ipv6 { port } {
> + return "\\\[::1\\\]:${port}"
> +}
> diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
> index df949994fd..9ec053c9d6 100644
> --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
> +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
> @@ -24,7 +24,12 @@ load_generic_config "extended-gdbserver"
> load_board_description "gdbserver-base"
> load_board_description "local-board"
>
> -set_board_info sockethost "localhost:"
> +if { [info exists GDB_TEST_IPV6] } {
> + set_board_info sockethost "tcp6:\[::1\]:"
> + set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
> +} else {
> + set_board_info sockethost "localhost:"
> +}
>
> # We will be using the extended GDB remote protocol.
> set_board_info gdb_protocol "extended-remote"
> diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
> index ef9316007e..d491aa451a 100644
> --- a/gdb/testsuite/boards/native-gdbserver.exp
> +++ b/gdb/testsuite/boards/native-gdbserver.exp
> @@ -30,7 +30,12 @@ set_board_info gdb,do_reload_on_run 1
> # There's no support for argument-passing (yet).
> set_board_info noargs 1
>
> -set_board_info sockethost "localhost:"
> +if { [info exists GDB_TEST_IPV6] } {
> + set_board_info sockethost "tcp6:\[::1\]:"
> + set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
> +} else {
> + set_board_info sockethost "localhost:"
> +}
> set_board_info use_gdb_stub 1
> set_board_info exit_is_reliable 1
>
> diff --git a/gdb/testsuite/gdb.server/run-without-local-binary.exp b/gdb/testsuite/gdb.server/run-without-local-binary.exp
> index 1665ca9912..6ba3e711d9 100644
> --- a/gdb/testsuite/gdb.server/run-without-local-binary.exp
> +++ b/gdb/testsuite/gdb.server/run-without-local-binary.exp
> @@ -53,7 +53,7 @@ save_vars { GDBFLAGS } {
> set use_gdb_stub 0
>
> gdb_test "target ${gdbserver_protocol} ${gdbserver_gdbport}" \
> - "Remote debugging using $gdbserver_gdbport" \
> + "Remote debugging using [string_to_regexp $gdbserver_gdbport]" \
> "connect to gdbserver"
>
> gdb_test "run" \
> --
> 2.14.3
Hi Sergio,
I noticed this when applying the patch:
$ git am /tmp/sergio.mbox
Applying: Implement IPv6 support for GDB/gdbserver
.git/rebase-apply/patch:982: trailing whitespace.
do
.git/rebase-apply/patch:985: trailing whitespace.
}
warning: 2 lines add whitespace errors.
You can check it locally with:
$ git show --check
gdb/ser-tcp.c:256: trailing whitespace.
+ do
gdb/ser-tcp.c:259: trailing whitespace.
+ }
Comments on the patch below.
>
> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
> parameter, which instructs GDB and gdbserver to use IPv6 for
> connections. This way, if you want to run IPv6 tests, you do:
>
> $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
That sounds useful, but:
#1 - I don't see how that works without also passing
--target_board= pointing at one of the native-gdbserver and
native-extended-gdbserver board files.
Can you expand on why you took this approach instead of:
a) handling GDB_TEST_IPV6 somewhere central, like
in gdb/testsuite/gdbserver-support.exp, where we
default to "localhost:". That would exercise the gdb.server/
tests with ipv6, when testing with the default/unix board file.
b) add new board files to test with ipv6, like native-gdbserver-v6
or something like that.
c) both?
#2 - I think it'd be also useful to have some gdb.server/ test that
runs with the default board and exercises / smoke tests ipv6.
(And if we have that, we might as well iterate the test on udp/udpv6
too.)
#3 - Actually, this makes me wonder about changing the variable's
spelling from GDB_TEST_IPV6=1 to something like
GDB_TEST_SOCKETHOST and then one would be able to set it to:
"localhost:",
"localhost6:"
"tcp:localhost6:"
"\[::1\]:"
"udp:127.0.0.1:"
or whatever one would like.
#4 - Why do we need to override get_comm_port too, here? :
-set_board_info sockethost "localhost:"
+if { [info exists GDB_TEST_IPV6] } {
+ set_board_info sockethost "tcp6:\[::1\]:"
+ set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
+} else {
+ set_board_info sockethost "localhost:"
+}
Doesn't overriding "sockethost" alone work? Why not?
> gdb/Makefile.in | 2 +
> gdb/NEWS | 4 +
> gdb/common/netstuff.c | 136 +++++++++++++
> gdb/common/netstuff.h | 52 +++++
> gdb/doc/gdb.texinfo | 48 ++++-
> gdb/gdbserver/Makefile.in | 2 +
> gdb/gdbserver/gdbreplay.c | 181 +++++++++++++----
> gdb/gdbserver/remote-utils.c | 119 +++++++----
> gdb/ser-tcp.c | 217 ++++++++++-----------
> gdb/testsuite/README | 7 +
> gdb/testsuite/boards/gdbserver-base.exp | 5 +
> gdb/testsuite/boards/native-extended-gdbserver.exp | 7 +-
> gdb/testsuite/boards/native-gdbserver.exp | 7 +-
> .../gdb.server/run-without-local-binary.exp | 2 +-
> 14 files changed, 602 insertions(+), 187 deletions(-)
> create mode 100644 gdb/common/netstuff.c
> create mode 100644 gdb/common/netstuff.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index df6ebab851..06ce12a4ee 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -962,6 +962,7 @@ COMMON_SFILES = \
> common/job-control.c \
> common/gdb_tilde_expand.c \
> common/gdb_vecs.c \
> + common/netstuff.c \
> common/new-op.c \
> common/pathstuff.c \
> common/print-utils.c \
> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
> common/gdb_vecs.h \
> common/gdb_wait.h \
> common/common-inferior.h \
> + common/netstuff.h \
> common/host-defs.h \
> common/pathstuff.h \
> common/print-utils.h \
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cef558039e..1f95ced912 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>
> *** Changes since GDB 8.1
>
> +* GDB and GDBserver now support IPv6 connections. IPv6 hostnames
> + can be passed using the '[ADDRESS]:PORT' notation, or the regular
> + 'ADDRESS:PORT' method.
Saying "IPv6 hostnames" and then talking about "ADDRESS:PORT" is
confusing, I think. If we're talking about host names then saying
HOST:PORT would more accurate. But I think that what you really
mean is to say "IPv6 _addresses_ can be passed".
Does connecting with "localhost6:port" default to IPv6, BTW?
At least fedora includes "localhost6" in /etc/hosts.
> +/* See common/netstuff.h. */
> +
> +void
> +parse_hostname_without_prefix (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint)
> +{
> + std::string strname (hostname);
I suspect the local parsing can be written using
gdb::string_view to avoid copying?
> +
> +/* See common/netstuff.h. */
> +
> +void
> +parse_hostname (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint)
> +{
> + /* Struct to hold the association between valid prefixes, their
> + family and socktype. */
> + struct host_prefix
> + {
> + /* The prefix. */
> + const char *prefix;
> +
> + /* The 'ai_family'. */
> + int family;
> +
> + /* The 'ai_socktype'. */
> + int socktype;
> + };
> + static const struct host_prefix prefixes[] =
> + {
> + { "udp:", AF_UNSPEC, SOCK_DGRAM },
> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
> + { "udp4:", AF_INET, SOCK_DGRAM },
> + { "tcp4:", AF_INET, SOCK_STREAM },
> + { "udp6:", AF_INET6, SOCK_DGRAM },
> + { "tcp6:", AF_INET6, SOCK_STREAM },
> + { NULL, 0, 0 },
> + };
> +
> + for (const struct host_prefix *prefix = prefixes;
> + prefix->prefix != NULL;
> + ++prefix)
I think you could drop the last/null entry and use range-for ?
> + if (startswith (hostname, prefix->prefix))
> + {
> + hostname += strlen (prefix->prefix);
> + hint->ai_family = prefix->family;
> + hint->ai_socktype = prefix->socktype;
> + hint->ai_protocol
> + = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
> + break;
> + }
> +
> + parse_hostname_without_prefix (hostname, host_str, port_str, hint);
> +}
> +/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill
Missing "form" in "in the of".
> + in HOST_STR, PORT_STR and HINT accordingly. */
> +extern void parse_hostname_without_prefix (const char *hostname,
> + std::string &host_str,
> + std::string &port_str,
> + struct addrinfo *hint);
> +
> +/* Parse HOSTNAME (which is a string in the form of
> + "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
> + HINT accordingly. */
> +extern void parse_hostname (const char *hostname, std::string &host_str,
> + std::string &port_str, struct addrinfo *hint);
Really not a big deal, but instead of output parameters, I'd
consider returning all outputs via return. Something like:
struct parsed_hostname
{
std::string host_str;
std::string port_str;
struct addrinfo addrinfo;
};
extern parsed_hostname parse_hostname (const char *hostname,
const struct addrinfo &hint);
> For example, to connect to port 2828 on a terminal server named
> @code{manyfarms}:
> @@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
> target remote manyfarms:2828
> @end smallexample
>
> +To connect to port 2828 on a terminal server whose address is
> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
> +
> +@smallexample
> +target remote [2001::f8ff::67cf]:2828
> +@end smallexample
> +
> +Or explicitly specify the @acronym{IPv6} protocol:
> +
> +@smallexample
> +target remote tcp6:2001::f8ff::67cf:2828
> +@end smallexample
> +
> +This last example may be confusing to the reader, because there is no
> +visible separation between the hostname and the port number.
Is that really true? It seems there's visible separation to me -- the
address/hosthoname part uses double colon, while the port name is
separated by a single colon?
> +Therefore, we recommend the user to provide @acronym{IPv6} addresses
> +using square brackets for clarity.
> +
> #ifndef HAVE_SOCKLEN_T
> @@ -175,56 +176,159 @@ remote_close (void)
> static void
> remote_open (char *name)
> {
> - if (!strchr (name, ':'))
> + char *last_colon = strrchr (name, ':');
> +
> + if (last_colon == NULL)
> {
> fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
> fflush (stderr);
> exit (1);
> }
> - else
> - {
> +
> #ifdef USE_WIN32API
> - static int winsock_initialized;
> + static int winsock_initialized;
> #endif
> - char *port_str;
> - int port;
> - struct sockaddr_in sockaddr;
> - socklen_t tmp;
> - int tmp_desc;
> + char *port_str;
> + int tmp;
> + int tmp_desc;
> + struct addrinfo hint;
> + struct addrinfo *ainfo;
> + char *orig_name = strdup (name);
Do we need a deep copy? And if we do, how about
using std::string to avoid having to call free further
down?
> +
> + struct prefix
> + {
> + /* The prefix to be parsed. */
> + const char *str;
> +
> + /* The address family. */
> + int ai_family;
> +
> + /* The socktype. */
> + int ai_socktype;
> + };
> + static const struct prefix prefixes[]
> + = { { "udp:", AF_UNSPEC, SOCK_DGRAM },
> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
> + { "udp4:", AF_INET, SOCK_DGRAM },
> + { "tcp4:", AF_INET, SOCK_STREAM },
> + { "udp6:", AF_INET6, SOCK_DGRAM },
> + { "tcp6:", AF_INET6, SOCK_STREAM },
> + { NULL, 0, 0 } };
That seems like unusual formatting. In common/netstuff.c
you broke the starting and ending '{' }' differently.
I wonder though, shouldn't this be using the new
netstuff.c shared routines? It looks like duplicated code?
> +
> + memset (&hint, 0, sizeof (hint));
> + /* Assume no prefix will be passed, therefore we should use
> + AF_UNSPEC. */
> + hint.ai_family = AF_UNSPEC;
> + hint.ai_socktype = SOCK_STREAM;
> + hint.ai_protocol = IPPROTO_TCP;
> +
> + for (const struct prefix *p = prefixes; p->str != NULL; ++p)
Same comment about range-for.
> +
> + if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
> + perror_with_name ("Can't bind address");
> +
> + if (p->ai_socktype == SOCK_DGRAM)
> + remote_desc = tmp_desc;
> + else
> + {
> + struct sockaddr_storage sockaddr;
> + socklen_t sockaddrsize = sizeof (sockaddr);
> + char orig_host[64], orig_port[16];
I guess these magic sizes are garanteed to be enough, since
you specify NI_NUMERICHOST | NI_NUMERICSERV. Correct?
A comment or giving those constants names or comments
would be good. Something like:
/* Like NI_MAXHOST/NI_MAXSERV, but enough for numeric forms. */
#define GDB_NI_MAX_ADDR 64
#define GDB_NI_MAX_PORT 16
> #if __QNX__
> @@ -156,19 +159,18 @@ enable_async_notification (int fd)
> static int
> handle_accept_event (int err, gdb_client_data client_data)
> {
> - struct sockaddr_in sockaddr;
> - socklen_t tmp;
> + struct sockaddr_storage sockaddr;
> + socklen_t len = sizeof (sockaddr);
>
> if (debug_threads)
> debug_printf ("handling possible accept event\n");
>
> - tmp = sizeof (sockaddr);
> - remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
> + remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
> if (remote_desc == -1)
> perror_with_name ("Accept failed");
>
> /* Enable TCP keep alive process. */
> - tmp = 1;
> + socklen_t tmp = 1;
> setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
> (char *) &tmp, sizeof (tmp));
>
> @@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
> delete_file_handler (listen_desc);
>
> /* Convert IP address to string. */
> - fprintf (stderr, "Remote debugging from host %s\n",
> - inet_ntoa (sockaddr.sin_addr));
> + char orig_host[64], orig_port[16];
Same comment as for gdbreplay.
> +
> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> + orig_host, sizeof (orig_host),
> + orig_port, sizeof (orig_port),
> + NI_NUMERICHOST | NI_NUMERICSERV);
> +
> + if (r != 0)
> + fprintf (stderr, _("Could not obtain remote address: %s\n"),
> + gai_strerror (r));
> + else
> + fprintf (stderr, "Remote debugging from host %s, port %s\n", orig_host,
> + orig_port);
While at it, couple you please add the missing _() for i18n.
BTW, is that line too long? Can't tell from email client.
>
> @@ -354,18 +399,24 @@ remote_open (const char *name)
> #endif /* USE_WIN32API */
> else
> {
> - int port;
> - socklen_t len;
> - struct sockaddr_in sockaddr;
> -
> - len = sizeof (sockaddr);
> - if (getsockname (listen_desc,
> - (struct sockaddr *) &sockaddr, &len) < 0
> - || len < sizeof (sockaddr))
> + char listen_port[16];
> + struct sockaddr_storage sockaddr;
> + socklen_t len = sizeof (sockaddr);
> +
> + if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
> perror_with_name ("Can't determine port");
> - port = ntohs (sockaddr.sin_port);
>
> - fprintf (stderr, "Listening on port %d\n", port);
> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> + NULL, 0,
> + listen_port, sizeof (listen_port),
> + NI_NUMERICSERV);
> +
> + if (r != 0)
> + fprintf (stderr, _("Can't obtain port where we are listening: %s"),
> + gai_strerror (r));
> + else
> + fprintf (stderr, "Listening on port %s\n", listen_port);
Preexisting, but while at it, adding the _() wouldn't hurt.
Thanks,
Pedro Alves
On Wednesday, June 06 2018, Pedro Alves wrote:
> Hi Sergio,
Hi Pedro,
Thanks for the review.
> I noticed this when applying the patch:
>
> $ git am /tmp/sergio.mbox
> Applying: Implement IPv6 support for GDB/gdbserver
> .git/rebase-apply/patch:982: trailing whitespace.
> do
> .git/rebase-apply/patch:985: trailing whitespace.
> }
> warning: 2 lines add whitespace errors.
>
> You can check it locally with:
>
> $ git show --check
>
> gdb/ser-tcp.c:256: trailing whitespace.
> + do
> gdb/ser-tcp.c:259: trailing whitespace.
> + }
>
> Comments on the patch below.
Yeah, I just reindented this region, without touching anything else.
I've now removed these trailing whitespaces.
>> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
>> parameter, which instructs GDB and gdbserver to use IPv6 for
>> connections. This way, if you want to run IPv6 tests, you do:
>>
>> $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
>
> That sounds useful, but:
>
> #1 - I don't see how that works without also passing
> --target_board= pointing at one of the native-gdbserver and
> native-extended-gdbserver board files.
> Can you expand on why you took this approach instead of:
>
> a) handling GDB_TEST_IPV6 somewhere central, like
> in gdb/testsuite/gdbserver-support.exp, where we
> default to "localhost:". That would exercise the gdb.server/
> tests with ipv6, when testing with the default/unix board file.
>
> b) add new board files to test with ipv6, like native-gdbserver-v6
> or something like that.
>
> c) both?
I was thinking about a good way to test this feature, and my initial
assumption was that the test would only make sense when --target-board=
is passed. That's why I chose to implement the mechanism on
gdb/testsuite/boards/gdbserver-base.exp. Now that you mentioned this, I
noticed that I should have also mentioned these expectations while
writing the commit message, and that the "make check-gdb
RUNTESTFLAGS='GDB_TEST_IPV6=1'" is actually wrong because it doesn't
specify any of the target boards.
Having said that, and after reading your question, I understand that the
testing can be made more flexible by implementing the logic inside
gdb/testsuite/gdbserver-support.exp instead, which will have the benefit
of activating the test even without a gdbserver target board being
specified. I will give it a try and see if I can implement it in a
better way.
> #2 - I think it'd be also useful to have some gdb.server/ test that
> runs with the default board and exercises / smoke tests ipv6.
> (And if we have that, we might as well iterate the test on udp/udpv6
> too.)
I thought about that, but I didn't know how to proceed when you don't
really know whether IPv6 support is present or not in the machine. I'll
give it another try.
> #3 - Actually, this makes me wonder about changing the variable's
> spelling from GDB_TEST_IPV6=1 to something like
> GDB_TEST_SOCKETHOST and then one would be able to set it to:
>
> "localhost:",
> "localhost6:"
> "tcp:localhost6:"
> "\[::1\]:"
> "udp:127.0.0.1:"
>
> or whatever one would like.
That works, and has the benefit of being protocol-agnostic. I'll
implement it.
> #4 - Why do we need to override get_comm_port too, here? :
>
> -set_board_info sockethost "localhost:"
> +if { [info exists GDB_TEST_IPV6] } {
> + set_board_info sockethost "tcp6:\[::1\]:"
> + set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
> +} else {
> + set_board_info sockethost "localhost:"
> +}
>
> Doesn't overriding "sockethost" alone work? Why not?
Hm. I can swear this wasn't working before (otherwise I wouldn't have
created more work just by unnecessarily implement this override), but
now that I commented it out, it's working again. This is
frustrating... Anyway, I'm going to rewrite a lot of this code, so this
part will disappear.
>> gdb/Makefile.in | 2 +
>> gdb/NEWS | 4 +
>> gdb/common/netstuff.c | 136 +++++++++++++
>> gdb/common/netstuff.h | 52 +++++
>> gdb/doc/gdb.texinfo | 48 ++++-
>> gdb/gdbserver/Makefile.in | 2 +
>> gdb/gdbserver/gdbreplay.c | 181 +++++++++++++----
>> gdb/gdbserver/remote-utils.c | 119 +++++++----
>> gdb/ser-tcp.c | 217 ++++++++++-----------
>> gdb/testsuite/README | 7 +
>> gdb/testsuite/boards/gdbserver-base.exp | 5 +
>> gdb/testsuite/boards/native-extended-gdbserver.exp | 7 +-
>> gdb/testsuite/boards/native-gdbserver.exp | 7 +-
>> .../gdb.server/run-without-local-binary.exp | 2 +-
>> 14 files changed, 602 insertions(+), 187 deletions(-)
>> create mode 100644 gdb/common/netstuff.c
>> create mode 100644 gdb/common/netstuff.h
>>
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index df6ebab851..06ce12a4ee 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -962,6 +962,7 @@ COMMON_SFILES = \
>> common/job-control.c \
>> common/gdb_tilde_expand.c \
>> common/gdb_vecs.c \
>> + common/netstuff.c \
>> common/new-op.c \
>> common/pathstuff.c \
>> common/print-utils.c \
>> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
>> common/gdb_vecs.h \
>> common/gdb_wait.h \
>> common/common-inferior.h \
>> + common/netstuff.h \
>> common/host-defs.h \
>> common/pathstuff.h \
>> common/print-utils.h \
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index cef558039e..1f95ced912 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,10 @@
>>
>> *** Changes since GDB 8.1
>>
>> +* GDB and GDBserver now support IPv6 connections. IPv6 hostnames
>> + can be passed using the '[ADDRESS]:PORT' notation, or the regular
>> + 'ADDRESS:PORT' method.
>
> Saying "IPv6 hostnames" and then talking about "ADDRESS:PORT" is
> confusing, I think. If we're talking about host names then saying
> HOST:PORT would more accurate. But I think that what you really
> mean is to say "IPv6 _addresses_ can be passed".
That's more correct, indeed. I've changed the text.
> Does connecting with "localhost6:port" default to IPv6, BTW?
> At least fedora includes "localhost6" in /etc/hosts.
Using "localhost6:port" works, but it doesn't default to IPv6. Here's
what I see on the gdbserver side:
$ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
Process /path/to/a.out created; pid = 7742
Listening on port 1234
Remote debugging from host ::ffff:127.0.0.1, port 39196
This means that the connection came using IPv4; it works because IPv6
sockets also listen for IPv4 connection on Linux (one can change this
behaviour by setting the "IPV6_V6ONLY" socket option).
This happens because I've made a decision to default to AF_INET (instead
of AF_UNSPEC) when no prefix has been given. This basically means that,
at least for now, we assume that an unknown (i.e., not prefixed)
address/hostname is IPv4. I've made this decision thinking about the
convenience of the user: when AF_UNSPEC is used (and the user hasn't
specified any prefix), getaddrinfo will return a linked list of possible
addresses that we should try to connect to, which usually means an IPv6
and an IPv4 address, in that order. Usually this is fine, because (as I
said) IPv6 sockets can also listen for IPv4 connections. However, if
you start gdbserver with an explicit IPv4 address:
$ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out
and try to connect GDB to it using an "ambiguous" hostname:
$ ./gdb/gdb -ex 'target remote localhost:1234' a.out
you will notice that GDB will take a somewhat long time trying to
connect (to the IPv6 address, because of AF_UNSPEC), and then it will
error out saying that the connection timed out:
tcp:localhost:1234: Connection timed out.
This is because of the auto-retry mechanism implemented for TCP
connections on GDB; it keeps retrying to connect to the IPv6 until it
decides it's not going to work. Only after this timeout is that GDB
will try to connect to the IPv4 address, and succeed.
So, the way I see it, we have a few options to deal with this scenario:
1) Assume that the unprefixed address/hostname is AF_INET (i.e., keep
the patch as-is).
2) Don't assume anything about the unprefixed address/hostname (i.e.,
AF_UNSPEC), and don't change the auto-retry system. This is not very
nice because of what I explained above.
3) Don't assume anything about the unprefixed address/hostname (i.e.,
AF_UNSPEC), but *DO* change the auto-retry system to retry less times
(currently it's set to 15 retries, which seems too much to me). Maybe 5
times is enough? This will still have an impact on the user, but she
will have to wait less time, at least.
Either (1) or (3) are fine by me. If we go with (1), we'll eventually
need to change the default to IPv6 (or to AF_UNSPEC), but that's only
when IPv6 is more adopted.
Side note: while I was writing this, I noticed a problem in the code and
fixed it. Basically, on net_open, should have been declared inside the
for loop, and not outside. This was making GDB not attempt connecting
the IPv4 address after the IPv6 failed. Now it's fixed.
>> +/* See common/netstuff.h. */
>> +
>> +void
>> +parse_hostname_without_prefix (const char *hostname, std::string &host_str,
>> + std::string &port_str, struct addrinfo *hint)
>> +{
>> + std::string strname (hostname);
>
> I suspect the local parsing can be written using
> gdb::string_view to avoid copying?
Hm, I'll investigate it. So many shiny new features available to us
now, it's hard to keep track!
>> +
>> +/* See common/netstuff.h. */
>> +
>> +void
>> +parse_hostname (const char *hostname, std::string &host_str,
>> + std::string &port_str, struct addrinfo *hint)
>> +{
>> + /* Struct to hold the association between valid prefixes, their
>> + family and socktype. */
>> + struct host_prefix
>> + {
>> + /* The prefix. */
>> + const char *prefix;
>> +
>> + /* The 'ai_family'. */
>> + int family;
>> +
>> + /* The 'ai_socktype'. */
>> + int socktype;
>> + };
>> + static const struct host_prefix prefixes[] =
>> + {
>> + { "udp:", AF_UNSPEC, SOCK_DGRAM },
>> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
>> + { "udp4:", AF_INET, SOCK_DGRAM },
>> + { "tcp4:", AF_INET, SOCK_STREAM },
>> + { "udp6:", AF_INET6, SOCK_DGRAM },
>> + { "tcp6:", AF_INET6, SOCK_STREAM },
>> + { NULL, 0, 0 },
>> + };
>> +
>> + for (const struct host_prefix *prefix = prefixes;
>> + prefix->prefix != NULL;
>> + ++prefix)
>
> I think you could drop the last/null entry and use range-for ?
I remember trying to use a rage-for but having issues. I think I was
using a different setup for this struct... Anyway, I implemented the
range-for idea now.
>> + if (startswith (hostname, prefix->prefix))
>> + {
>> + hostname += strlen (prefix->prefix);
>> + hint->ai_family = prefix->family;
>> + hint->ai_socktype = prefix->socktype;
>> + hint->ai_protocol
>> + = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
>> + break;
>> + }
>> +
>> + parse_hostname_without_prefix (hostname, host_str, port_str, hint);
>> +}
>
>
>> +/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill
>
> Missing "form" in "in the of".
Fixed.
>> + in HOST_STR, PORT_STR and HINT accordingly. */
>> +extern void parse_hostname_without_prefix (const char *hostname,
>> + std::string &host_str,
>> + std::string &port_str,
>> + struct addrinfo *hint);
>> +
>> +/* Parse HOSTNAME (which is a string in the form of
>> + "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
>> + HINT accordingly. */
>> +extern void parse_hostname (const char *hostname, std::string &host_str,
>> + std::string &port_str, struct addrinfo *hint);
>
> Really not a big deal, but instead of output parameters, I'd
> consider returning all outputs via return. Something like:
>
> struct parsed_hostname
> {
> std::string host_str;
> std::string port_str;
> struct addrinfo addrinfo;
> };
> extern parsed_hostname parse_hostname (const char *hostname,
> const struct addrinfo &hint);
Sure, I can do it.
>> For example, to connect to port 2828 on a terminal server named
>> @code{manyfarms}:
>> @@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
>> target remote manyfarms:2828
>> @end smallexample
>>
>> +To connect to port 2828 on a terminal server whose address is
>> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
>> +
>> +@smallexample
>> +target remote [2001::f8ff::67cf]:2828
>> +@end smallexample
>> +
>> +Or explicitly specify the @acronym{IPv6} protocol:
>> +
>> +@smallexample
>> +target remote tcp6:2001::f8ff::67cf:2828
>> +@end smallexample
>> +
>> +This last example may be confusing to the reader, because there is no
>> +visible separation between the hostname and the port number.
>
> Is that really true? It seems there's visible separation to me -- the
> address/hosthoname part uses double colon, while the port name is
> separated by a single colon?
Ah, I think that's not a good address to use as an example. I'll use a
"regular" address without double colons.
>> +Therefore, we recommend the user to provide @acronym{IPv6} addresses
>> +using square brackets for clarity.
>> +
>
>> #ifndef HAVE_SOCKLEN_T
>> @@ -175,56 +176,159 @@ remote_close (void)
>> static void
>> remote_open (char *name)
>> {
>> - if (!strchr (name, ':'))
>> + char *last_colon = strrchr (name, ':');
>> +
>> + if (last_colon == NULL)
>> {
>> fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
>> fflush (stderr);
>> exit (1);
>> }
>> - else
>> - {
>> +
>> #ifdef USE_WIN32API
>> - static int winsock_initialized;
>> + static int winsock_initialized;
>> #endif
>> - char *port_str;
>> - int port;
>> - struct sockaddr_in sockaddr;
>> - socklen_t tmp;
>> - int tmp_desc;
>> + char *port_str;
>> + int tmp;
>> + int tmp_desc;
>> + struct addrinfo hint;
>> + struct addrinfo *ainfo;
>> + char *orig_name = strdup (name);
>
> Do we need a deep copy? And if we do, how about
> using std::string to avoid having to call free further
> down?
This is gdbserver/gdbreplay.c, where apparently we don't have access to
a lot of our regular facilities on GDB. For example, I was trying to
use std::string, its methods, and other stuff here (even i18n
functions), but the code won't compile, and as far as I have researched
this is intentional, because gdbreplay needs to be a very small and
simple program. Or at least that's what I understood from our
archives/documentation. I did not feel confident reworking gdbreplay to
make it "modern", so I decided to implement things "the old way".
>> +
>> + struct prefix
>> + {
>> + /* The prefix to be parsed. */
>> + const char *str;
>> +
>> + /* The address family. */
>> + int ai_family;
>> +
>> + /* The socktype. */
>> + int ai_socktype;
>> + };
>> + static const struct prefix prefixes[]
>> + = { { "udp:", AF_UNSPEC, SOCK_DGRAM },
>> + { "tcp:", AF_UNSPEC, SOCK_STREAM },
>> + { "udp4:", AF_INET, SOCK_DGRAM },
>> + { "tcp4:", AF_INET, SOCK_STREAM },
>> + { "udp6:", AF_INET6, SOCK_DGRAM },
>> + { "tcp6:", AF_INET6, SOCK_STREAM },
>> + { NULL, 0, 0 } };
>
> That seems like unusual formatting. In common/netstuff.c
> you broke the starting and ending '{' }' differently.
I'll improve the formatting here.
> I wonder though, shouldn't this be using the new
> netstuff.c shared routines? It looks like duplicated code?
The reason here is the same as the one I wrote above: doing '#include
"netstuff.h"' brings in a lot of other stuff which break the build of
gdbreplay.c. That's why I reimplemented the code (and yeah, it is code
duplication, and I wasn't happy about it, but as I said it feels that
these limitations are intentional on gdbreplay.c).
>> +
>> + memset (&hint, 0, sizeof (hint));
>> + /* Assume no prefix will be passed, therefore we should use
>> + AF_UNSPEC. */
>> + hint.ai_family = AF_UNSPEC;
>> + hint.ai_socktype = SOCK_STREAM;
>> + hint.ai_protocol = IPPROTO_TCP;
>> +
>> + for (const struct prefix *p = prefixes; p->str != NULL; ++p)
>
> Same comment about range-for.
Fixed.
>> +
>> + if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
>> + perror_with_name ("Can't bind address");
>> +
>> + if (p->ai_socktype == SOCK_DGRAM)
>> + remote_desc = tmp_desc;
>> + else
>> + {
>> + struct sockaddr_storage sockaddr;
>> + socklen_t sockaddrsize = sizeof (sockaddr);
>> + char orig_host[64], orig_port[16];
>
> I guess these magic sizes are garanteed to be enough, since
> you specify NI_NUMERICHOST | NI_NUMERICSERV. Correct?
> A comment or giving those constants names or comments
> would be good. Something like:
>
> /* Like NI_MAXHOST/NI_MAXSERV, but enough for numeric forms. */
> #define GDB_NI_MAX_ADDR 64
> #define GDB_NI_MAX_PORT 16
Good idea, I'll do it.
>> #if __QNX__
>> @@ -156,19 +159,18 @@ enable_async_notification (int fd)
>> static int
>> handle_accept_event (int err, gdb_client_data client_data)
>> {
>> - struct sockaddr_in sockaddr;
>> - socklen_t tmp;
>> + struct sockaddr_storage sockaddr;
>> + socklen_t len = sizeof (sockaddr);
>>
>> if (debug_threads)
>> debug_printf ("handling possible accept event\n");
>>
>> - tmp = sizeof (sockaddr);
>> - remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
>> + remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
>> if (remote_desc == -1)
>> perror_with_name ("Accept failed");
>>
>> /* Enable TCP keep alive process. */
>> - tmp = 1;
>> + socklen_t tmp = 1;
>> setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
>> (char *) &tmp, sizeof (tmp));
>>
>> @@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
>> delete_file_handler (listen_desc);
>>
>> /* Convert IP address to string. */
>> - fprintf (stderr, "Remote debugging from host %s\n",
>> - inet_ntoa (sockaddr.sin_addr));
>> + char orig_host[64], orig_port[16];
>
> Same comment as for gdbreplay.
Consider it done.
>> +
>> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
>> + orig_host, sizeof (orig_host),
>> + orig_port, sizeof (orig_port),
>> + NI_NUMERICHOST | NI_NUMERICSERV);
>> +
>> + if (r != 0)
>> + fprintf (stderr, _("Could not obtain remote address: %s\n"),
>> + gai_strerror (r));
>> + else
>> + fprintf (stderr, "Remote debugging from host %s, port %s\n", orig_host,
>> + orig_port);
>
> While at it, couple you please add the missing _() for i18n.
Done.
> BTW, is that line too long? Can't tell from email client.
78 chars with _() added. I've decided to break it anyway.
>>
>> @@ -354,18 +399,24 @@ remote_open (const char *name)
>> #endif /* USE_WIN32API */
>> else
>> {
>> - int port;
>> - socklen_t len;
>> - struct sockaddr_in sockaddr;
>> -
>> - len = sizeof (sockaddr);
>> - if (getsockname (listen_desc,
>> - (struct sockaddr *) &sockaddr, &len) < 0
>> - || len < sizeof (sockaddr))
>> + char listen_port[16];
>> + struct sockaddr_storage sockaddr;
>> + socklen_t len = sizeof (sockaddr);
>> +
>> + if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
>> perror_with_name ("Can't determine port");
>> - port = ntohs (sockaddr.sin_port);
>>
>> - fprintf (stderr, "Listening on port %d\n", port);
>> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
>> + NULL, 0,
>> + listen_port, sizeof (listen_port),
>> + NI_NUMERICSERV);
>> +
>> + if (r != 0)
>> + fprintf (stderr, _("Can't obtain port where we are listening: %s"),
>> + gai_strerror (r));
>> + else
>> + fprintf (stderr, "Listening on port %s\n", listen_port);
>
> Preexisting, but while at it, adding the _() wouldn't hurt.
Done.
On 06/08/2018 02:13 AM, Sergio Durigan Junior wrote:
> On Wednesday, June 06 2018, Pedro Alves wrote:
>>> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
>>> parameter, which instructs GDB and gdbserver to use IPv6 for
>>> connections. This way, if you want to run IPv6 tests, you do:
>>>
>>> $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
>>
>> That sounds useful, but:
>>
>> #1 - I don't see how that works without also passing
>> --target_board= pointing at one of the native-gdbserver and
>> native-extended-gdbserver board files.
>> Can you expand on why you took this approach instead of:
>>
>> a) handling GDB_TEST_IPV6 somewhere central, like
>> in gdb/testsuite/gdbserver-support.exp, where we
>> default to "localhost:". That would exercise the gdb.server/
>> tests with ipv6, when testing with the default/unix board file.
>>
>> b) add new board files to test with ipv6, like native-gdbserver-v6
>> or something like that.
>>
>> c) both?
>
> I was thinking about a good way to test this feature, and my initial
> assumption was that the test would only make sense when --target-board=
> is passed. That's why I chose to implement the mechanism on
> gdb/testsuite/boards/gdbserver-base.exp. Now that you mentioned this, I
> noticed that I should have also mentioned these expectations while
> writing the commit message, and that the "make check-gdb
> RUNTESTFLAGS='GDB_TEST_IPV6=1'" is actually wrong because it doesn't
> specify any of the target boards.
>
> Having said that, and after reading your question, I understand that the
> testing can be made more flexible by implementing the logic inside
> gdb/testsuite/gdbserver-support.exp instead, which will have the benefit
> of activating the test even without a gdbserver target board being
> specified. I will give it a try and see if I can implement it in a
> better way.
I'd think you just have to hook the GDB_TEST_LOCALHOST env var reading here,
in gdbserver_start:
# Extract the local and remote host ids from the target board struct.
if [target_info exists sockethost] {
set debughost [target_info sockethost]
} else {
set debughost "localhost:"
}
I'd also try removing the
set_board_info sockethost "localhost:"
line from native-gdbserver.exp and native-extended-gdbserver.exp,
since that's the default. But it's not really necessary if
the env var takes precedence of the target board setting.
>> Does connecting with "localhost6:port" default to IPv6, BTW?
>> At least fedora includes "localhost6" in /etc/hosts.
>
> Using "localhost6:port" works, but it doesn't default to IPv6. Here's
> what I see on the gdbserver side:
>
> $ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
> Process /path/to/a.out created; pid = 7742
> Listening on port 1234
> Remote debugging from host ::ffff:127.0.0.1, port 39196
>
> This means that the connection came using IPv4; it works because IPv6
> sockets also listen for IPv4 connection on Linux (one can change this
> behaviour by setting the "IPV6_V6ONLY" socket option).
>
> This happens because I've made a decision to default to AF_INET (instead
> of AF_UNSPEC) when no prefix has been given. This basically means that,
> at least for now, we assume that an unknown (i.e., not prefixed)
> address/hostname is IPv4. I've made this decision thinking about the
> convenience of the user: when AF_UNSPEC is used (and the user hasn't
> specified any prefix), getaddrinfo will return a linked list of possible
> addresses that we should try to connect to, which usually means an IPv6
> and an IPv4 address, in that order. Usually this is fine, because (as I
> said) IPv6 sockets can also listen for IPv4 connections. However, if
> you start gdbserver with an explicit IPv4 address:
>
> $ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out
>
> and try to connect GDB to it using an "ambiguous" hostname:
>
> $ ./gdb/gdb -ex 'target remote localhost:1234' a.out
>
> you will notice that GDB will take a somewhat long time trying to
> connect (to the IPv6 address, because of AF_UNSPEC), and then it will
> error out saying that the connection timed out:
>
> tcp:localhost:1234: Connection timed out.
How do other tools handle this? For example, with ping, I get:
$ ping localhost
PING localhost.localdomain (127.0.0.1) 56(84) bytes of data.
64 bytes from localhost.localdomain (127.0.0.1): icmp_seq=1 ttl=64 time=0.048 ms
^C
$ ping localhost6
PING localhost6(localhost6.localdomain6 (::1)) 56 data bytes
64 bytes from localhost6.localdomain6 (::1): icmp_seq=1 ttl=64 time=0.086 ms
^C
how does ping instantly know without visible delay that "localhost"
resolves to an IPv4 address, and that "localhost6" resolves to
an IPv6 address?
Same with telnet:
$ telnet localhost
Trying 127.0.0.1...
telnet: connect to address 127.0.0.1: Connection refused
$ telnet localhost6
Trying ::1...
telnet: connect to address ::1: Connection refused
Same with netcat:
$ nc -vv localhost
Ncat: Version 7.60 ( https://nmap.org/ncat )
NCAT DEBUG: Using system default trusted CA certificates and those in /usr/share/ncat/ca-bundle.crt.
NCAT DEBUG: Unable to load trusted CA certificates from /usr/share/ncat/ca-bundle.crt: error:02001002:system library:fopen:No such file or directory
libnsock nsock_iod_new2(): nsock_iod_new (IOD #1)
libnsock nsock_connect_tcp(): TCP connection requested to 127.0.0.1:31337 (IOD #1) EID 8
^^^^^^^^^
libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 8 [127.0.0.1:31337]
Ncat: Connection refused.
$ nc -vv localhost6
Ncat: Version 7.60 ( https://nmap.org/ncat )
NCAT DEBUG: Using system default trusted CA certificates and those in /usr/share/ncat/ca-bundle.crt.
NCAT DEBUG: Unable to load trusted CA certificates from /usr/share/ncat/ca-bundle.crt: error:02001002:system library:fopen:No such file or directory
libnsock nsock_iod_new2(): nsock_iod_new (IOD #1)
libnsock nsock_connect_tcp(): TCP connection requested to ::1:31337 (IOD #1) EID 8
^^^
libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 8 [::1:31337]
Ncat: Connection refused.
[
BTW, I think a much more common scenario of local use of
gdbserver is to omit the host name:
./gdb/gdbserver/gdbserver --once :1234
./gdb/gdb -ex 'target remote :1234'
I assume that would work fine with AF_UNSPEC ?
>
> This is because of the auto-retry mechanism implemented for TCP
> connections on GDB; it keeps retrying to connect to the IPv6 until it
> decides it's not going to work. Only after this timeout is that GDB
> will try to connect to the IPv4 address, and succeed.
>
> So, the way I see it, we have a few options to deal with this scenario:
>
> 1) Assume that the unprefixed address/hostname is AF_INET (i.e., keep
> the patch as-is).
>
> 2) Don't assume anything about the unprefixed address/hostname (i.e.,
> AF_UNSPEC), and don't change the auto-retry system. This is not very
> nice because of what I explained above.
>
> 3) Don't assume anything about the unprefixed address/hostname (i.e.,
> AF_UNSPEC), but *DO* change the auto-retry system to retry less times
> (currently it's set to 15 retries, which seems too much to me). Maybe 5
> times is enough? This will still have an impact on the user, but she
> will have to wait less time, at least.
>
> Either (1) or (3) are fine by me. If we go with (1), we'll eventually
> need to change the default to IPv6 (or to AF_UNSPEC), but that's only
> when IPv6 is more adopted.
I'd like to understand this a bit more before coming up with a
decision. I feel like we're missing something.
A part of it is that it kind of looks like a "it hurts when I do this
doctor; then just don't" scenario, with the using different host names
on both gdbserver and gdb (localhost vs 127.0.0.1). Why would you do that
for local debugging? You'd get the same problem if localhost
always only resolved to an IPv6 address, I'd think. But still, I'd
like to understand how can other tools handle this.
>>> + char *orig_name = strdup (name);
>>
>> Do we need a deep copy? And if we do, how about
>> using std::string to avoid having to call free further
>> down?
>
> This is gdbserver/gdbreplay.c, where apparently we don't have access to
> a lot of our regular facilities on GDB. For example, I was trying to
> use std::string, its methods, and other stuff here (even i18n
> functions), but the code won't compile, and as far as I have researched
> this is intentional, because gdbreplay needs to be a very small and
> simple program.
What did you find that gave you that impression? There's no reason
that gdbreplay needs to be small or simple. Certainly doesn't need
to be smaller than gdbserver.
> at least that's what I understood from our
> archives/documentation. I did not feel confident reworking gdbreplay to
> make it "modern", so I decided to implement things "the old way".
Seems like adding to technical debt to be honest. Did you hit some
unsurmountable problem, or would just a little bit of fixing here and
there be doable?
Thanks,
Pedro Alves
On Friday, June 08 2018, Pedro Alves wrote:
> On 06/08/2018 02:13 AM, Sergio Durigan Junior wrote:
>> On Wednesday, June 06 2018, Pedro Alves wrote:
>
>
>>>> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
>>>> parameter, which instructs GDB and gdbserver to use IPv6 for
>>>> connections. This way, if you want to run IPv6 tests, you do:
>>>>
>>>> $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
>>>
>>> That sounds useful, but:
>>>
>>> #1 - I don't see how that works without also passing
>>> --target_board= pointing at one of the native-gdbserver and
>>> native-extended-gdbserver board files.
>>> Can you expand on why you took this approach instead of:
>>>
>>> a) handling GDB_TEST_IPV6 somewhere central, like
>>> in gdb/testsuite/gdbserver-support.exp, where we
>>> default to "localhost:". That would exercise the gdb.server/
>>> tests with ipv6, when testing with the default/unix board file.
>>>
>>> b) add new board files to test with ipv6, like native-gdbserver-v6
>>> or something like that.
>>>
>>> c) both?
>>
>> I was thinking about a good way to test this feature, and my initial
>> assumption was that the test would only make sense when --target-board=
>> is passed. That's why I chose to implement the mechanism on
>> gdb/testsuite/boards/gdbserver-base.exp. Now that you mentioned this, I
>> noticed that I should have also mentioned these expectations while
>> writing the commit message, and that the "make check-gdb
>> RUNTESTFLAGS='GDB_TEST_IPV6=1'" is actually wrong because it doesn't
>> specify any of the target boards.
>>
>> Having said that, and after reading your question, I understand that the
>> testing can be made more flexible by implementing the logic inside
>> gdb/testsuite/gdbserver-support.exp instead, which will have the benefit
>> of activating the test even without a gdbserver target board being
>> specified. I will give it a try and see if I can implement it in a
>> better way.
>
> I'd think you just have to hook the GDB_TEST_LOCALHOST env var reading here,
> in gdbserver_start:
>
> # Extract the local and remote host ids from the target board struct.
> if [target_info exists sockethost] {
> set debughost [target_info sockethost]
> } else {
> set debughost "localhost:"
> }
Yes, that's my plan.
> I'd also try removing the
>
> set_board_info sockethost "localhost:"
>
> line from native-gdbserver.exp and native-extended-gdbserver.exp,
> since that's the default. But it's not really necessary if
> the env var takes precedence of the target board setting.
I wasn't planning to remove these lines from the board files, but I can
do it.
>>> Does connecting with "localhost6:port" default to IPv6, BTW?
>>> At least fedora includes "localhost6" in /etc/hosts.
>>
>> Using "localhost6:port" works, but it doesn't default to IPv6. Here's
>> what I see on the gdbserver side:
>>
>> $ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
>> Process /path/to/a.out created; pid = 7742
>> Listening on port 1234
>> Remote debugging from host ::ffff:127.0.0.1, port 39196
>>
>> This means that the connection came using IPv4; it works because IPv6
>> sockets also listen for IPv4 connection on Linux (one can change this
>> behaviour by setting the "IPV6_V6ONLY" socket option).
>>
>> This happens because I've made a decision to default to AF_INET (instead
>> of AF_UNSPEC) when no prefix has been given. This basically means that,
>> at least for now, we assume that an unknown (i.e., not prefixed)
>> address/hostname is IPv4. I've made this decision thinking about the
>> convenience of the user: when AF_UNSPEC is used (and the user hasn't
>> specified any prefix), getaddrinfo will return a linked list of possible
>> addresses that we should try to connect to, which usually means an IPv6
>> and an IPv4 address, in that order. Usually this is fine, because (as I
>> said) IPv6 sockets can also listen for IPv4 connections. However, if
>> you start gdbserver with an explicit IPv4 address:
>>
>> $ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out
>>
>> and try to connect GDB to it using an "ambiguous" hostname:
>>
>> $ ./gdb/gdb -ex 'target remote localhost:1234' a.out
>>
>> you will notice that GDB will take a somewhat long time trying to
>> connect (to the IPv6 address, because of AF_UNSPEC), and then it will
>> error out saying that the connection timed out:
>>
>> tcp:localhost:1234: Connection timed out.
>
> How do other tools handle this?
Just like GDB.
> For example, with ping, I get:
>
> $ ping localhost
> PING localhost.localdomain (127.0.0.1) 56(84) bytes of data.
> 64 bytes from localhost.localdomain (127.0.0.1): icmp_seq=1 ttl=64 time=0.048 ms
> ^C
>
> $ ping localhost6
> PING localhost6(localhost6.localdomain6 (::1)) 56 data bytes
> 64 bytes from localhost6.localdomain6 (::1): icmp_seq=1 ttl=64 time=0.086 ms
> ^C
And I get:
$ ping localhost
PING localhost(localhost (::1)) 56 data bytes
64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.050 ms
^C
$ ping localhost6
PING localhost6(localhost (::1)) 56 data bytes
64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.089 ms
^C
Maybe your /etc/hosts is different than mine:
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
> how does ping instantly know without visible delay that "localhost"
> resolves to an IPv4 address, and that "localhost6" resolves to
> an IPv6 address?
It doesn't. It just tries to connect to the first entry returned by
getaddrinfo:
https://github.com/iputils/iputils/blob/master/ping.c#L518
In my case, since both IPv4 and IPv6 localhost addresses are valid, it
just connects to the first one, which is the IPv6.
> Same with telnet:
>
> $ telnet localhost
> Trying 127.0.0.1...
> telnet: connect to address 127.0.0.1: Connection refused
> $ telnet localhost6
> Trying ::1...
> telnet: connect to address ::1: Connection refused
Using telnet allows one to see the algorithm I explained before working:
$ telnet localhost
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
telnet: connect to address 127.0.0.1: Connection refused
It tried to connect to the IPv6 address, got a connection refused (which
happened instantly, without delay, because telnet doesn't implement our
tcp-retry thing), then it tried to connect to the IPv4 address, which
also failed. This is the getaddrinfo loop working.
https://github.com/marado/netkit-telnet/blob/596f1a79d3868f733fc15765b107638822a0f8a9/netkit-telnet-0.17/telnet/commands.cc#L1808
> Same with netcat:
>
> $ nc -vv localhost
> Ncat: Version 7.60 ( https://nmap.org/ncat )
> NCAT DEBUG: Using system default trusted CA certificates and those in /usr/share/ncat/ca-bundle.crt.
> NCAT DEBUG: Unable to load trusted CA certificates from /usr/share/ncat/ca-bundle.crt: error:02001002:system library:fopen:No such file or directory
> libnsock nsock_iod_new2(): nsock_iod_new (IOD #1)
> libnsock nsock_connect_tcp(): TCP connection requested to 127.0.0.1:31337 (IOD #1) EID 8
> ^^^^^^^^^
> libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 8 [127.0.0.1:31337]
> Ncat: Connection refused.
>
> $ nc -vv localhost6
> Ncat: Version 7.60 ( https://nmap.org/ncat )
> NCAT DEBUG: Using system default trusted CA certificates and those in /usr/share/ncat/ca-bundle.crt.
> NCAT DEBUG: Unable to load trusted CA certificates from /usr/share/ncat/ca-bundle.crt: error:02001002:system library:fopen:No such file or directory
> libnsock nsock_iod_new2(): nsock_iod_new (IOD #1)
> libnsock nsock_connect_tcp(): TCP connection requested to ::1:31337 (IOD #1) EID 8
> ^^^
> libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 8 [::1:31337]
> Ncat: Connection refused.
> [
$ nc -vv localhost
Ncat: Version 7.60 ( https://nmap.org/ncat )
NCAT DEBUG: Using system default trusted CA certificates and those in /usr/share/ncat/ca-bundle.crt.
NCAT DEBUG: Unable to load trusted CA certificates from /usr/share/ncat/ca-bundle.crt: error:02001002:system library:fopen:No such file or directory
libnsock nsock_iod_new2(): nsock_iod_new (IOD #1)
libnsock nsock_connect_tcp(): TCP connection requested to ::1:31337 (IOD #1) EID 8
^^^
libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 8 [::1:31337]
Ncat: Connection to ::1 failed: Connection refused.
Ncat: Trying next address...
^^^^^^^^^^^^^^^^^^^
libnsock nsock_connect_tcp(): TCP connection requested to 127.0.0.1:31337 (IOD #1) EID 16
^^^^^^^^^
libnsock nsock_trace_handler_callback(): Callback: CONNECT ERROR [Connection refused (111)] for EID 16 [127.0.0.1:31337]
Ncat: Connection refused.
> BTW, I think a much more common scenario of local use of
> gdbserver is to omit the host name:
>
> ./gdb/gdbserver/gdbserver --once :1234
> ./gdb/gdb -ex 'target remote :1234'
>
> I assume that would work fine with AF_UNSPEC ?
Yes, with AF_UNSPEC we assume the hostname is "localhost" if it's not
given, and it works like it should (i.e., IPv6 first, IPv4 later).
>>
>> This is because of the auto-retry mechanism implemented for TCP
>> connections on GDB; it keeps retrying to connect to the IPv6 until it
>> decides it's not going to work. Only after this timeout is that GDB
>> will try to connect to the IPv4 address, and succeed.
>>
>> So, the way I see it, we have a few options to deal with this scenario:
>>
>> 1) Assume that the unprefixed address/hostname is AF_INET (i.e., keep
>> the patch as-is).
>>
>> 2) Don't assume anything about the unprefixed address/hostname (i.e.,
>> AF_UNSPEC), and don't change the auto-retry system. This is not very
>> nice because of what I explained above.
>>
>> 3) Don't assume anything about the unprefixed address/hostname (i.e.,
>> AF_UNSPEC), but *DO* change the auto-retry system to retry less times
>> (currently it's set to 15 retries, which seems too much to me). Maybe 5
>> times is enough? This will still have an impact on the user, but she
>> will have to wait less time, at least.
>>
>> Either (1) or (3) are fine by me. If we go with (1), we'll eventually
>> need to change the default to IPv6 (or to AF_UNSPEC), but that's only
>> when IPv6 is more adopted.
>
> I'd like to understand this a bit more before coming up with a
> decision. I feel like we're missing something.
>
> A part of it is that it kind of looks like a "it hurts when I do this
> doctor; then just don't" scenario, with the using different host names
> on both gdbserver and gdb (localhost vs 127.0.0.1). Why would you do that
> for local debugging? You'd get the same problem if localhost
> always only resolved to an IPv6 address, I'd think. But still, I'd
> like to understand how can other tools handle this.
The "getaddrinfo loop" is a well known way to implement IPv6 support on
IPv4-only tools. I think it is totally fine to iterate through the
possible addresses and try each one until we have success, but our
problem is that we implemented a retry mechanism on top of that, so when
we get "connection refused" GDB won't just try the next address, but
will keep retrying the refused one... That is the problem.
I don't know why would anyone use different hostnames on both GDB and
gdbserver, I just stated the fact that if someone does it, she will have
problems. And yes, you'd get the same problem if localhost always only
resolved to IPv6. The difference is that the tools you're using for
your example don't implement retry (or at least not the way we do), so
you don't have huge delays when they can't connect to an address.
It is clear to me, after investigating this, that the problem is our
retry mechanism. We can either adjust it to a lower delay, get rid of
it, or leave it as is and assume that unprefixed addresses are IPv4. I
fail to see what else we're missing.
>>>> + char *orig_name = strdup (name);
>>>
>>> Do we need a deep copy? And if we do, how about
>>> using std::string to avoid having to call free further
>>> down?
>>
>> This is gdbserver/gdbreplay.c, where apparently we don't have access to
>> a lot of our regular facilities on GDB. For example, I was trying to
>> use std::string, its methods, and other stuff here (even i18n
>> functions), but the code won't compile, and as far as I have researched
>> this is intentional, because gdbreplay needs to be a very small and
>> simple program.
>
> What did you find that gave you that impression? There's no reason
> that gdbreplay needs to be small or simple. Certainly doesn't need
> to be smaller than gdbserver.
First, the way it is written. It doesn't use any of our facilities
(e.g., i18n, strdup instead of xstrdup), and it seems to be treated in a
"special" way, because it is a separate program. I found this message:
https://sourceware.org/ml/gdb/2008-06/msg00117.html
> I've tried to find information in the doc about gdbreplay without luck.
> Really quickly, does gdbreplay, as its name suggest, allow to record an
> re-run an application session?
Yes, exactly -- but with rather stringent limits.
In a nutshell, during the replay session, you must give
EXACTLY the same sequence of gdb commands as were given
during the record session. gdbreplay will prompt you for
the next command, but if you do *anything* different,
it will throw up its hands and quit.
And it seems to imply that gdbreplay is a very limited program. And
Jan's first patch (back in 2006) implementing IPv6 also duplicated code
on gdbreplay. I admit I may have read too much between the lines here,
but I just assumed that this was just the way things were.
>> at least that's what I understood from our
>> archives/documentation. I did not feel confident reworking gdbreplay to
>> make it "modern", so I decided to implement things "the old way".
>
> Seems like adding to technical debt to be honest. Did you hit some
> unsurmountable problem, or would just a little bit of fixing here and
> there be doable?
I don't know if it's unsurmountable. I know I had trouble getting i18n
and trying to include a few headers here and there, but I haven't tried
very hard to work around it. I just decided to "add to the technical
debt".
I'll take a better look at this.
Thanks,
On 06/08/2018 06:47 PM, Sergio Durigan Junior wrote:
> On Friday, June 08 2018, Pedro Alves wrote:
>>>> Does connecting with "localhost6:port" default to IPv6, BTW?
>>>> At least fedora includes "localhost6" in /etc/hosts.
>>>
>>> Using "localhost6:port" works, but it doesn't default to IPv6. Here's
>>> what I see on the gdbserver side:
>>>
>>> $ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
>>> Process /path/to/a.out created; pid = 7742
>>> Listening on port 1234
>>> Remote debugging from host ::ffff:127.0.0.1, port 39196
>>>
>>> This means that the connection came using IPv4; it works because IPv6
>>> sockets also listen for IPv4 connection on Linux (one can change this
>>> behaviour by setting the "IPV6_V6ONLY" socket option).
>>>
>>> This happens because I've made a decision to default to AF_INET (instead
>>> of AF_UNSPEC) when no prefix has been given. This basically means that,
>>> at least for now, we assume that an unknown (i.e., not prefixed)
>>> address/hostname is IPv4. I've made this decision thinking about the
>>> convenience of the user: when AF_UNSPEC is used (and the user hasn't
>>> specified any prefix), getaddrinfo will return a linked list of possible
>>> addresses that we should try to connect to, which usually means an IPv6
>>> and an IPv4 address, in that order. Usually this is fine, because (as I
>>> said) IPv6 sockets can also listen for IPv4 connections. However, if
>>> you start gdbserver with an explicit IPv4 address:
>>>
>>> $ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out
>>>
>>> and try to connect GDB to it using an "ambiguous" hostname:
>>>
>>> $ ./gdb/gdb -ex 'target remote localhost:1234' a.out
>>>
>>> you will notice that GDB will take a somewhat long time trying to
>>> connect (to the IPv6 address, because of AF_UNSPEC), and then it will
>>> error out saying that the connection timed out:
>>>
>>> tcp:localhost:1234: Connection timed out.
>>
>> How do other tools handle this?
>
> Just like GDB.
Well, it sounds like they do the AF_UNSPEC thing, instead of
defaulting to AF_INET.
>
>
>> For example, with ping, I get:
>>
>> $ ping localhost
>> PING localhost.localdomain (127.0.0.1) 56(84) bytes of data.
>> 64 bytes from localhost.localdomain (127.0.0.1): icmp_seq=1 ttl=64 time=0.048 ms
>> ^C
>>
>> $ ping localhost6
>> PING localhost6(localhost6.localdomain6 (::1)) 56 data bytes
>> 64 bytes from localhost6.localdomain6 (::1): icmp_seq=1 ttl=64 time=0.086 ms
>> ^C
>
> And I get:
>
> $ ping localhost
> PING localhost(localhost (::1)) 56 data bytes
> 64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.050 ms
> ^C
>
> $ ping localhost6
> PING localhost6(localhost (::1)) 56 data bytes
> 64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.089 ms
> ^C
>
> Maybe your /etc/hosts is different than mine:
>
> 127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
> ::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
Looks like it:
$ cat /etc/hosts
127.0.0.1 localhost.localdomain localhost
::1 localhost6.localdomain6 localhost6
This is on F27 (upgraded a few times). I don't remember if I ever
changed this manually.
>
>> how does ping instantly know without visible delay that "localhost"
>> resolves to an IPv4 address, and that "localhost6" resolves to
>> an IPv6 address?
>
> It doesn't. It just tries to connect to the first entry returned by
> getaddrinfo:
>
> https://github.com/iputils/iputils/blob/master/ping.c#L518
>
> In my case, since both IPv4 and IPv6 localhost addresses are valid, it
> just connects to the first one, which is the IPv6.
OK.
> Using telnet allows one to see the algorithm I explained before working:
OK.
> The "getaddrinfo loop" is a well known way to implement IPv6 support on
> IPv4-only tools. I think it is totally fine to iterate through the
> possible addresses and try each one until we have success, but our
> problem is that we implemented a retry mechanism on top of that, so when
> we get "connection refused" GDB won't just try the next address, but
> will keep retrying the refused one... That is the problem.
Yes.
> I don't know why would anyone use different hostnames on both GDB and
> gdbserver, I just stated the fact that if someone does it, she will have
> problems. And yes, you'd get the same problem if localhost always only
> resolved to IPv6. The difference is that the tools you're using for
> your example don't implement retry (or at least not the way we do), so
> you don't have huge delays when they can't connect to an address.
>
> It is clear to me, after investigating this, that the problem is our
> retry mechanism.
Agreed, it's clear to me too.
> We can either adjust it to a lower delay, get rid of
> it, or leave it as is and assume that unprefixed addresses are IPv4. I
> fail to see what else we're missing.
The "assume unprefixed addresses are IPv4" seems like the worse
option to me, as it's a work around. Let's tackle the real issue
instead.
We could consider for example more verbose progress indication,
or cycling the whole "getaddrinfo loop" before waiting to retry instead
of waiting after each individual connection failure.
>
>>>>> + char *orig_name = strdup (name);
>>>>
>>>> Do we need a deep copy? And if we do, how about
>>>> using std::string to avoid having to call free further
>>>> down?
>>>
>>> This is gdbserver/gdbreplay.c, where apparently we don't have access to
>>> a lot of our regular facilities on GDB. For example, I was trying to
>>> use std::string, its methods, and other stuff here (even i18n
>>> functions), but the code won't compile, and as far as I have researched
>>> this is intentional, because gdbreplay needs to be a very small and
>>> simple program.
>>
>> What did you find that gave you that impression? There's no reason
>> that gdbreplay needs to be small or simple. Certainly doesn't need
>> to be smaller than gdbserver.
>
> First, the way it is written. It doesn't use any of our facilities
> (e.g., i18n, strdup instead of xstrdup), and it seems to be treated in a
> "special" way, because it is a separate program. I found this message:
>
> https://sourceware.org/ml/gdb/2008-06/msg00117.html
>
> > I've tried to find information in the doc about gdbreplay without luck.
> > Really quickly, does gdbreplay, as its name suggest, allow to record an
> > re-run an application session?
>
> Yes, exactly -- but with rather stringent limits.
> In a nutshell, during the replay session, you must give
> EXACTLY the same sequence of gdb commands as were given
> during the record session. gdbreplay will prompt you for
> the next command, but if you do *anything* different,
> it will throw up its hands and quit.
>
> And it seems to imply that gdbreplay is a very limited program.
The "stringent limits" refers to what you can do from gdb
when connected to gdbreplay, not to gdbreplay's running environment.
The tool is useful for support, to e.g., reproduce bugs that only
trigger against remote stubs / embedded probes that the user has
access to, other than gdbserver. You ask the user to use "set remotelogfile"
to record the remote protocol traffic against the user's remote stub, and
then the user reproduces the bug. The user sends you the resulting
remote protocol log file, and you load it against gdbreplay. Then, using
the same gdb version and same program binary the user used, you connect
to gdbreplay, and use the exact same set of gdb commands the user used.
Given those constraints, gdb should send the exact same remote protocol
packets that the user's gdb sent to the users stub. And since gdbreplay
is replaying the remote protocol traffic the original stub sent, you'll be
able to reproduce the bug locally. If you use a different set of commands,
then gdb will send different packets, and the log that gdbreplay is
replaying of course breaks down.
So gdbreplay runs on the host computer. There's no need to care about
making it super tiny of anything like that.
And>
> Jan's first patch (back in 2006) implementing IPv6 also duplicated code
> on gdbreplay. I admit I may have read too much between the lines here,
> but I just assumed that this was just the way things were.
>
>>> at least that's what I understood from our
>>> archives/documentation. I did not feel confident reworking gdbreplay to
>>> make it "modern", so I decided to implement things "the old way".
>>
>> Seems like adding to technical debt to be honest. Did you hit some
>> unsurmountable problem, or would just a little bit of fixing here and
>> there be doable?
>
> I don't know if it's unsurmountable. I know I had trouble getting i18n
> and trying to include a few headers here and there, but I haven't tried
> very hard to work around it. I just decided to "add to the technical
> debt".
>
> I'll take a better look at this.
Thanks.
Pedro Alves
On 06/08/2018 07:44 PM, Pedro Alves wrote:
>> Jan's first patch (back in 2006) implementing IPv6 also duplicated code
>> on gdbreplay. I admit I may have read too much between the lines here,
>> but I just assumed that this was just the way things were.
>>
>>>> at least that's what I understood from our
>>>> archives/documentation. I did not feel confident reworking gdbreplay to
>>>> make it "modern", so I decided to implement things "the old way".
>>> Seems like adding to technical debt to be honest. Did you hit some
>>> unsurmountable problem, or would just a little bit of fixing here and
>>> there be doable?
>> I don't know if it's unsurmountable. I know I had trouble getting i18n
>> and trying to include a few headers here and there, but I haven't tried
>> very hard to work around it. I just decided to "add to the technical
>> debt".
>>
>> I'll take a better look at this.
> Thanks.
I took a quick look at it. I'll send a patch.
Thanks,
Pedro Alves
On 06/08/2018 08:28 PM, Pedro Alves wrote:
> I took a quick look at it. I'll send a patch.
Done <https://sourceware.org/ml/gdb-patches/2018-06/msg00232.html>.
I first wrote that on top of your patch, and made sure that
including "netstuff.h" worked. Hopefully all you'll need to
do is add netstuff.o in the Makefile and whatever dependencies
that might have (hopefully none or not many).
Thanks,
Pedro Alves
On Friday, June 08 2018, Pedro Alves wrote:
> On 06/08/2018 08:28 PM, Pedro Alves wrote:
>
>> I took a quick look at it. I'll send a patch.
>
> Done <https://sourceware.org/ml/gdb-patches/2018-06/msg00232.html>.
>
> I first wrote that on top of your patch, and made sure that
> including "netstuff.h" worked. Hopefully all you'll need to
> do is add netstuff.o in the Makefile and whatever dependencies
> that might have (hopefully none or not many).
Thanks, much appreciated, Pedro.
On Friday, June 08 2018, Pedro Alves wrote:
> On 06/08/2018 06:47 PM, Sergio Durigan Junior wrote:
>> On Friday, June 08 2018, Pedro Alves wrote:
>
>>>>> Does connecting with "localhost6:port" default to IPv6, BTW?
>>>>> At least fedora includes "localhost6" in /etc/hosts.
>>>>
>>>> Using "localhost6:port" works, but it doesn't default to IPv6. Here's
>>>> what I see on the gdbserver side:
>>>>
>>>> $ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
>>>> Process /path/to/a.out created; pid = 7742
>>>> Listening on port 1234
>>>> Remote debugging from host ::ffff:127.0.0.1, port 39196
>>>>
>>>> This means that the connection came using IPv4; it works because IPv6
>>>> sockets also listen for IPv4 connection on Linux (one can change this
>>>> behaviour by setting the "IPV6_V6ONLY" socket option).
>>>>
>>>> This happens because I've made a decision to default to AF_INET (instead
>>>> of AF_UNSPEC) when no prefix has been given. This basically means that,
>>>> at least for now, we assume that an unknown (i.e., not prefixed)
>>>> address/hostname is IPv4. I've made this decision thinking about the
>>>> convenience of the user: when AF_UNSPEC is used (and the user hasn't
>>>> specified any prefix), getaddrinfo will return a linked list of possible
>>>> addresses that we should try to connect to, which usually means an IPv6
>>>> and an IPv4 address, in that order. Usually this is fine, because (as I
>>>> said) IPv6 sockets can also listen for IPv4 connections. However, if
>>>> you start gdbserver with an explicit IPv4 address:
>>>>
>>>> $ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out
>>>>
>>>> and try to connect GDB to it using an "ambiguous" hostname:
>>>>
>>>> $ ./gdb/gdb -ex 'target remote localhost:1234' a.out
>>>>
>>>> you will notice that GDB will take a somewhat long time trying to
>>>> connect (to the IPv6 address, because of AF_UNSPEC), and then it will
>>>> error out saying that the connection timed out:
>>>>
>>>> tcp:localhost:1234: Connection timed out.
>>>
>>> How do other tools handle this?
>>
>> Just like GDB.
>
> Well, it sounds like they do the AF_UNSPEC thing, instead of
> defaulting to AF_INET.
Yeah. Sorry, I was comparing with the GDB I currently have here, which
uses AF_UNSPEC because I was doing a few tests and changes.
>>> For example, with ping, I get:
>>>
>>> $ ping localhost
>>> PING localhost.localdomain (127.0.0.1) 56(84) bytes of data.
>>> 64 bytes from localhost.localdomain (127.0.0.1): icmp_seq=1 ttl=64 time=0.048 ms
>>> ^C
>>>
>>> $ ping localhost6
>>> PING localhost6(localhost6.localdomain6 (::1)) 56 data bytes
>>> 64 bytes from localhost6.localdomain6 (::1): icmp_seq=1 ttl=64 time=0.086 ms
>>> ^C
>>
>> And I get:
>>
>> $ ping localhost
>> PING localhost(localhost (::1)) 56 data bytes
>> 64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.050 ms
>> ^C
>>
>> $ ping localhost6
>> PING localhost6(localhost (::1)) 56 data bytes
>> 64 bytes from localhost (::1): icmp_seq=1 ttl=64 time=0.089 ms
>> ^C
>>
>> Maybe your /etc/hosts is different than mine:
>>
>> 127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
>> ::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
>
> Looks like it:
>
> $ cat /etc/hosts
> 127.0.0.1 localhost.localdomain localhost
> ::1 localhost6.localdomain6 localhost6
>
> This is on F27 (upgraded a few times). I don't remember if I ever
> changed this manually.
I've never changed mine either, but this is from a fresh F27 install.
>> We can either adjust it to a lower delay, get rid of
>> it, or leave it as is and assume that unprefixed addresses are IPv4. I
>> fail to see what else we're missing.
>
> The "assume unprefixed addresses are IPv4" seems like the worse
> option to me, as it's a work around. Let's tackle the real issue
> instead.
>
> We could consider for example more verbose progress indication,
> or cycling the whole "getaddrinfo loop" before waiting to retry instead
> of waiting after each individual connection failure.
A more verbose indication would be nice, as well as a way to control how
many retries GDB should perform.
I'm not sure about cycling the whole loop before waiting to retry... I
mean, it works, but I'm not sure it's what the user would expect from a
"retry" mechanism. I would expect GDB to "retry this address X times,
and then go to the next", instead of "retry all the addresses in a
loop". But that can be documented, sure.
On a side note, I have to ask: why not decrease the number of retries
to, say, 5? This would still add a delay, but it'd be much shorter.
>>
>>>>>> + char *orig_name = strdup (name);
>>>>>
>>>>> Do we need a deep copy? And if we do, how about
>>>>> using std::string to avoid having to call free further
>>>>> down?
>>>>
>>>> This is gdbserver/gdbreplay.c, where apparently we don't have access to
>>>> a lot of our regular facilities on GDB. For example, I was trying to
>>>> use std::string, its methods, and other stuff here (even i18n
>>>> functions), but the code won't compile, and as far as I have researched
>>>> this is intentional, because gdbreplay needs to be a very small and
>>>> simple program.
>>>
>>> What did you find that gave you that impression? There's no reason
>>> that gdbreplay needs to be small or simple. Certainly doesn't need
>>> to be smaller than gdbserver.
>>
>> First, the way it is written. It doesn't use any of our facilities
>> (e.g., i18n, strdup instead of xstrdup), and it seems to be treated in a
>> "special" way, because it is a separate program. I found this message:
>>
>> https://sourceware.org/ml/gdb/2008-06/msg00117.html
>>
>> > I've tried to find information in the doc about gdbreplay without luck.
>> > Really quickly, does gdbreplay, as its name suggest, allow to record an
>> > re-run an application session?
>>
>> Yes, exactly -- but with rather stringent limits.
>> In a nutshell, during the replay session, you must give
>> EXACTLY the same sequence of gdb commands as were given
>> during the record session. gdbreplay will prompt you for
>> the next command, but if you do *anything* different,
>> it will throw up its hands and quit.
>>
>> And it seems to imply that gdbreplay is a very limited program.
>
> The "stringent limits" refers to what you can do from gdb
> when connected to gdbreplay, not to gdbreplay's running environment.
Right, I got that. As I said, I think I read too much between the lines
here.
> The tool is useful for support, to e.g., reproduce bugs that only
> trigger against remote stubs / embedded probes that the user has
> access to, other than gdbserver. You ask the user to use "set remotelogfile"
> to record the remote protocol traffic against the user's remote stub, and
> then the user reproduces the bug. The user sends you the resulting
> remote protocol log file, and you load it against gdbreplay. Then, using
> the same gdb version and same program binary the user used, you connect
> to gdbreplay, and use the exact same set of gdb commands the user used.
> Given those constraints, gdb should send the exact same remote protocol
> packets that the user's gdb sent to the users stub. And since gdbreplay
> is replaying the remote protocol traffic the original stub sent, you'll be
> able to reproduce the bug locally. If you use a different set of commands,
> then gdb will send different packets, and the log that gdbreplay is
> replaying of course breaks down.
>
> So gdbreplay runs on the host computer. There's no need to care about
> making it super tiny of anything like that.
We should add this to a wiki or to our official docs. I think it's the
more complete explanation I've read about gdbreplay so far.
Thanks,
On 06/08/2018 10:21 PM, Sergio Durigan Junior wrote:
> On Friday, June 08 2018, Pedro Alves wrote:
>>> We can either adjust it to a lower delay, get rid of
>>> it, or leave it as is and assume that unprefixed addresses are IPv4. I
>>> fail to see what else we're missing.
>>
>> The "assume unprefixed addresses are IPv4" seems like the worse
>> option to me, as it's a work around. Let's tackle the real issue
>> instead.
>>
>> We could consider for example more verbose progress indication,
>> or cycling the whole "getaddrinfo loop" before waiting to retry instead
>> of waiting after each individual connection failure.
>
> A more verbose indication would be nice, as well as a way to control how
> many retries GDB should perform.
>
> I'm not sure about cycling the whole loop before waiting to retry... I
> mean, it works, but I'm not sure it's what the user would expect from a
> "retry" mechanism. I would expect GDB to "retry this address X times,
> and then go to the next", instead of "retry all the addresses in a
> loop". But that can be documented, sure.
Cycling the whole loop seems to me the best option. The retry mechanism
exists because:
@item set tcp auto-retry on
@cindex auto-retry, for remote TCP target
Enable auto-retry for remote TCP connections. This is useful if the remote
debugging agent is launched in parallel with @value{GDBN}; there is a race
condition because the agent may not become ready to accept the connection
before @value{GDBN} attempts to connect. When auto-retry is
enabled, if the initial attempt to connect fails, @value{GDBN} reattempts
to establish the connection using the timeout specified by
@code{set tcp connect-timeout}.
If we cycle the whole loop before retrying we end up with a tiny tiny race
window where gdb may have tried IPv6, that failing because gdbserver was not
listening yet, and then gdb trying IPv4 and that succeeding. In that rare
scenario, if gdb had started looping just a fraction of a second earlier, it
would have connected with IPv6 successfully. But, so what? It will have connected
successfully anyway, and IPv6 vs IPv4 will hardly make a real difference.
Users that really really really want to ensure to get IPv6 or IPv4 should use
the "tcp6:" or "tcp4:" prefixes. So I'm not seeing any downside the whole loop
approach.
>
> On a side note, I have to ask: why not decrease the number of retries
> to, say, 5? This would still add a delay, but it'd be much shorter.
Because the solution above sounds much better to me. No delay.
> We should add this to a wiki or to our official docs. I think it's the
> more complete explanation I've read about gdbreplay so far.
That might be a good idea.
Thanks,
Pedro Alves
On Friday, June 08 2018, Pedro Alves wrote:
> On 06/08/2018 10:21 PM, Sergio Durigan Junior wrote:
>> On Friday, June 08 2018, Pedro Alves wrote:
>
>>>> We can either adjust it to a lower delay, get rid of
>>>> it, or leave it as is and assume that unprefixed addresses are IPv4. I
>>>> fail to see what else we're missing.
>>>
>>> The "assume unprefixed addresses are IPv4" seems like the worse
>>> option to me, as it's a work around. Let's tackle the real issue
>>> instead.
>>>
>>> We could consider for example more verbose progress indication,
>>> or cycling the whole "getaddrinfo loop" before waiting to retry instead
>>> of waiting after each individual connection failure.
>>
>> A more verbose indication would be nice, as well as a way to control how
>> many retries GDB should perform.
>>
>> I'm not sure about cycling the whole loop before waiting to retry... I
>> mean, it works, but I'm not sure it's what the user would expect from a
>> "retry" mechanism. I would expect GDB to "retry this address X times,
>> and then go to the next", instead of "retry all the addresses in a
>> loop". But that can be documented, sure.
>
> Cycling the whole loop seems to me the best option. The retry mechanism
> exists because:
>
> @item set tcp auto-retry on
> @cindex auto-retry, for remote TCP target
> Enable auto-retry for remote TCP connections. This is useful if the remote
> debugging agent is launched in parallel with @value{GDBN}; there is a race
> condition because the agent may not become ready to accept the connection
> before @value{GDBN} attempts to connect. When auto-retry is
> enabled, if the initial attempt to connect fails, @value{GDBN} reattempts
> to establish the connection using the timeout specified by
> @code{set tcp connect-timeout}.
>
> If we cycle the whole loop before retrying we end up with a tiny tiny race
> window where gdb may have tried IPv6, that failing because gdbserver was not
> listening yet, and then gdb trying IPv4 and that succeeding. In that rare
> scenario, if gdb had started looping just a fraction of a second earlier, it
> would have connected with IPv6 successfully. But, so what? It will have connected
> successfully anyway, and IPv6 vs IPv4 will hardly make a real difference.
> Users that really really really want to ensure to get IPv6 or IPv4 should use
> the "tcp6:" or "tcp4:" prefixes. So I'm not seeing any downside the whole loop
> approach.
OK, I don't really mind enough to argue. I'll implement it this way.
@@ -962,6 +962,7 @@ COMMON_SFILES = \
common/job-control.c \
common/gdb_tilde_expand.c \
common/gdb_vecs.c \
+ common/netstuff.c \
common/new-op.c \
common/pathstuff.c \
common/print-utils.c \
@@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
common/gdb_vecs.h \
common/gdb_wait.h \
common/common-inferior.h \
+ common/netstuff.h \
common/host-defs.h \
common/pathstuff.h \
common/print-utils.h \
@@ -3,6 +3,10 @@
*** Changes since GDB 8.1
+* GDB and GDBserver now support IPv6 connections. IPv6 hostnames
+ can be passed using the '[ADDRESS]:PORT' notation, or the regular
+ 'ADDRESS:PORT' method.
+
* The commands 'info variables/functions/types' now show the source line
numbers of symbol definitions when available.
new file mode 100644
@@ -0,0 +1,136 @@
+/* Operations on network stuff.
+ Copyright (C) 2018 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "common-defs.h"
+#include "netstuff.h"
+
+#ifdef USE_WIN32API
+#include <winsock2.h>
+#include <wspiapi.h>
+#else
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <netdb.h>
+#include <sys/socket.h>
+#include <netinet/tcp.h>
+#endif
+
+/* See common/netstuff.h. */
+
+scoped_free_addrinfo::scoped_free_addrinfo (struct addrinfo *a)
+{
+ m_res = a;
+}
+/* See common/netstuff.h. */
+
+scoped_free_addrinfo::~scoped_free_addrinfo ()
+{
+ freeaddrinfo (m_res);
+}
+
+/* See common/netstuff.h. */
+
+void
+parse_hostname_without_prefix (const char *hostname, std::string &host_str,
+ std::string &port_str, struct addrinfo *hint)
+{
+ std::string strname (hostname);
+
+ if (hint->ai_family != AF_INET && strname[0] == '[')
+ {
+ /* IPv6 addresses can be written as '[ADDR]:PORT', and we
+ support this notation. */
+ size_t close_bracket_pos = strname.find_first_of (']');
+
+ if (close_bracket_pos == std::string::npos)
+ error (_("Missing close bracket in hostname '%s'"),
+ strname.c_str ());
+
+ /* Erase both '[' and ']'. */
+ strname.erase (0, 1);
+ strname.erase (close_bracket_pos - 1, 1);
+
+ hint->ai_family = AF_INET6;
+ }
+
+ /* The length of the hostname part. */
+ size_t host_len;
+
+ size_t last_colon_pos = strname.find_last_of (':');
+
+ if (last_colon_pos != std::string::npos)
+ {
+ /* The user has provided a port. */
+ host_len = last_colon_pos;
+ port_str = strname.substr (last_colon_pos + 1);
+ }
+ else
+ host_len = strname.size ();
+
+ host_str = strname.substr (0, host_len);
+
+ /* Default hostname is localhost. */
+ if (host_str.empty ())
+ host_str = "localhost";
+}
+
+/* See common/netstuff.h. */
+
+void
+parse_hostname (const char *hostname, std::string &host_str,
+ std::string &port_str, struct addrinfo *hint)
+{
+ /* Struct to hold the association between valid prefixes, their
+ family and socktype. */
+ struct host_prefix
+ {
+ /* The prefix. */
+ const char *prefix;
+
+ /* The 'ai_family'. */
+ int family;
+
+ /* The 'ai_socktype'. */
+ int socktype;
+ };
+ static const struct host_prefix prefixes[] =
+ {
+ { "udp:", AF_UNSPEC, SOCK_DGRAM },
+ { "tcp:", AF_UNSPEC, SOCK_STREAM },
+ { "udp4:", AF_INET, SOCK_DGRAM },
+ { "tcp4:", AF_INET, SOCK_STREAM },
+ { "udp6:", AF_INET6, SOCK_DGRAM },
+ { "tcp6:", AF_INET6, SOCK_STREAM },
+ { NULL, 0, 0 },
+ };
+
+ for (const struct host_prefix *prefix = prefixes;
+ prefix->prefix != NULL;
+ ++prefix)
+ if (startswith (hostname, prefix->prefix))
+ {
+ hostname += strlen (prefix->prefix);
+ hint->ai_family = prefix->family;
+ hint->ai_socktype = prefix->socktype;
+ hint->ai_protocol
+ = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
+ break;
+ }
+
+ parse_hostname_without_prefix (hostname, host_str, port_str, hint);
+}
new file mode 100644
@@ -0,0 +1,52 @@
+/* Operations on network stuff.
+ Copyright (C) 2018 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef NETSTUFF_H
+#define NETSTUFF_H
+
+#include <string>
+
+/* Helper class to guarantee that we always call 'freeaddrinfo'. */
+
+class scoped_free_addrinfo
+{
+public:
+ scoped_free_addrinfo (struct addrinfo *a);
+
+ ~scoped_free_addrinfo ();
+
+ DISABLE_COPY_AND_ASSIGN (scoped_free_addrinfo);
+
+private:
+ struct addrinfo *m_res;
+};
+
+/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill
+ in HOST_STR, PORT_STR and HINT accordingly. */
+extern void parse_hostname_without_prefix (const char *hostname,
+ std::string &host_str,
+ std::string &port_str,
+ struct addrinfo *hint);
+
+/* Parse HOSTNAME (which is a string in the form of
+ "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
+ HINT accordingly. */
+extern void parse_hostname (const char *hostname, std::string &host_str,
+ std::string &port_str, struct addrinfo *hint);
+
+#endif /* ! NETSTUFF_H */
@@ -20496,16 +20496,27 @@ If you're using a serial line, you may want to give @value{GDBN} the
@code{target} command.
@item target remote @code{@var{host}:@var{port}}
+@itemx target remote @code{@var{@r{[}host@r{]}}:@var{port}}
@itemx target remote @code{tcp:@var{host}:@var{port}}
+@itemx target remote @code{tcp:@var{@r{[}host@r{]}}:@var{port}}
+@itemx target remote @code{tcp4:@var{host}:@var{port}}
+@itemx target remote @code{tcp6:@var{host}:@var{port}}
+@itemx target remote @code{tcp6:@var{@r{[}host@r{]}}:@var{port}}
@itemx target extended-remote @code{@var{host}:@var{port}}
+@itemx target extended-remote @code{@var{@r{[}host@r{]}}:@var{port}}
@itemx target extended-remote @code{tcp:@var{host}:@var{port}}
+@itemx target extended-remote @code{tcp:@var{@r{[}host@r{]}}:@var{port}}
+@itemx target extended-remote @code{tcp4:@var{host}:@var{port}}
+@itemx target extended-remote @code{tcp6:@var{host}:@var{port}}
+@itemx target extended-remote @code{tcp6:@var{@r{[}host@r{]}}:@var{port}}
@cindex @acronym{TCP} port, @code{target remote}
Debug using a @acronym{TCP} connection to @var{port} on @var{host}.
-The @var{host} may be either a host name or a numeric @acronym{IP}
-address; @var{port} must be a decimal number. The @var{host} could be
-the target machine itself, if it is directly connected to the net, or
-it might be a terminal server which in turn has a serial line to the
-target.
+The @var{host} may be either a host name, a numeric @acronym{IPv4}
+address, or a numeric @acronym{IPv6} address (with or without the
+square brackets to separate the address from the port); @var{port}
+must be a decimal number. The @var{host} could be the target machine
+itself, if it is directly connected to the net, or it might be a
+terminal server which in turn has a serial line to the target.
For example, to connect to port 2828 on a terminal server named
@code{manyfarms}:
@@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
target remote manyfarms:2828
@end smallexample
+To connect to port 2828 on a terminal server whose address is
+@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
+
+@smallexample
+target remote [2001::f8ff::67cf]:2828
+@end smallexample
+
+Or explicitly specify the @acronym{IPv6} protocol:
+
+@smallexample
+target remote tcp6:2001::f8ff::67cf:2828
+@end smallexample
+
+This last example may be confusing to the reader, because there is no
+visible separation between the hostname and the port number.
+Therefore, we recommend the user to provide @acronym{IPv6} addresses
+using square brackets for clarity.
+
If your remote target is actually running on the same machine as your
debugger session (e.g.@: a simulator for your target running on the
same host), you can omit the hostname. For example, to connect to
@@ -20527,7 +20556,16 @@ target remote :1234
Note that the colon is still required here.
@item target remote @code{udp:@var{host}:@var{port}}
+@itemx target remote @code{udp:@var{host}:@var{port}}
+@itemx target remote @code{udp:@var{@r{[}host@r{]}}:@var{port}}
+@itemx target remote @code{udp4:@var{host}:@var{port}}
+@itemx target remote @code{udp6:@var{@r{[}host@r{]}}:@var{port}}
+@itemx target extended-remote @code{udp:@var{host}:@var{port}}
@itemx target extended-remote @code{udp:@var{host}:@var{port}}
+@itemx target extended-remote @code{udp:@var{@r{[}host@r{]}}:@var{port}}
+@itemx target extended-remote @code{udp4:@var{host}:@var{port}}
+@itemx target extended-remote @code{udp6:@var{host}:@var{port}}
+@itemx target extended-remote @code{udp6:@var{@r{[}host@r{]}}:@var{port}}
@cindex @acronym{UDP} port, @code{target remote}
Debug using @acronym{UDP} packets to @var{port} on @var{host}. For example, to
connect to @acronym{UDP} port 2828 on a terminal server named @code{manyfarms}:
@@ -211,6 +211,7 @@ SFILES = \
$(srcdir)/common/job-control.c \
$(srcdir)/common/gdb_tilde_expand.c \
$(srcdir)/common/gdb_vecs.c \
+ $(srcdir)/common/netstuff.c \
$(srcdir)/common/new-op.c \
$(srcdir)/common/pathstuff.c \
$(srcdir)/common/print-utils.c \
@@ -253,6 +254,7 @@ OBS = \
common/format.o \
common/gdb_tilde_expand.o \
common/gdb_vecs.o \
+ common/netstuff.o \
common/new-op.o \
common/pathstuff.o \
common/print-utils.o \
@@ -53,6 +53,7 @@
#if USE_WIN32API
#include <winsock2.h>
+#include <wspiapi.h>
#endif
#ifndef HAVE_SOCKLEN_T
@@ -175,56 +176,159 @@ remote_close (void)
static void
remote_open (char *name)
{
- if (!strchr (name, ':'))
+ char *last_colon = strrchr (name, ':');
+
+ if (last_colon == NULL)
{
fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
fflush (stderr);
exit (1);
}
- else
- {
+
#ifdef USE_WIN32API
- static int winsock_initialized;
+ static int winsock_initialized;
#endif
- char *port_str;
- int port;
- struct sockaddr_in sockaddr;
- socklen_t tmp;
- int tmp_desc;
+ char *port_str;
+ int tmp;
+ int tmp_desc;
+ struct addrinfo hint;
+ struct addrinfo *ainfo;
+ char *orig_name = strdup (name);
+
+ struct prefix
+ {
+ /* The prefix to be parsed. */
+ const char *str;
+
+ /* The address family. */
+ int ai_family;
+
+ /* The socktype. */
+ int ai_socktype;
+ };
+ static const struct prefix prefixes[]
+ = { { "udp:", AF_UNSPEC, SOCK_DGRAM },
+ { "tcp:", AF_UNSPEC, SOCK_STREAM },
+ { "udp4:", AF_INET, SOCK_DGRAM },
+ { "tcp4:", AF_INET, SOCK_STREAM },
+ { "udp6:", AF_INET6, SOCK_DGRAM },
+ { "tcp6:", AF_INET6, SOCK_STREAM },
+ { NULL, 0, 0 } };
+
+ memset (&hint, 0, sizeof (hint));
+ /* Assume no prefix will be passed, therefore we should use
+ AF_UNSPEC. */
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_socktype = SOCK_STREAM;
+ hint.ai_protocol = IPPROTO_TCP;
+
+ for (const struct prefix *p = prefixes; p->str != NULL; ++p)
+ if (strncmp (name, p->str, strlen (p->str)) == 0)
+ {
+ name += strlen (p->str);
+ hint.ai_family = p->ai_family;
+ hint.ai_socktype = p->ai_socktype;
+ hint.ai_protocol
+ = p->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
+ break;
+ }
- port_str = strchr (name, ':');
+ if (hint.ai_family != AF_INET && *name == '[')
+ {
+ ++name;
+ port_str = strchr (name, ']');
+ if (port_str == NULL)
+ {
+ fprintf (stderr, "Missing closing bracket on hostname: %s\n",
+ orig_name);
+ exit (1);
+ }
+ /* Skip closing bracket. */
+ *port_str = '\0';
+ ++port_str;
+ if (*port_str != ':')
+ {
+ fprintf (stderr, "Missing port number on hostname: %s\n",
+ orig_name);
+ exit (1);
+ }
+ hint.ai_family = AF_INET6;
+ }
+ else
+ port_str = last_colon;
- port = atoi (port_str + 1);
+ /* Skip the colon. */
+ *port_str = '\0';
+ ++port_str;
#ifdef USE_WIN32API
- if (!winsock_initialized)
- {
- WSADATA wsad;
+ if (!winsock_initialized)
+ {
+ WSADATA wsad;
- WSAStartup (MAKEWORD (1, 0), &wsad);
- winsock_initialized = 1;
- }
+ WSAStartup (MAKEWORD (1, 0), &wsad);
+ winsock_initialized = 1;
+ }
#endif
- tmp_desc = socket (PF_INET, SOCK_STREAM, 0);
- if (tmp_desc == -1)
- perror_with_name ("Can't open socket");
+ int r = getaddrinfo (name, port_str, &hint, &ainfo);
- /* Allow rapid reuse of this port. */
- tmp = 1;
- setsockopt (tmp_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
- sizeof (tmp));
+ if (r != 0)
+ {
+ fprintf (stderr, "%s:%s: cannot resolve name: %s\n",
+ name, port_str, gai_strerror (r));
+ fflush (stderr);
+ exit (1);
+ }
- sockaddr.sin_family = PF_INET;
- sockaddr.sin_port = htons (port);
- sockaddr.sin_addr.s_addr = INADDR_ANY;
+ struct addrinfo *p;
- if (bind (tmp_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
- || listen (tmp_desc, 1))
- perror_with_name ("Can't bind address");
+ for (p = ainfo; p != NULL; p = p->ai_next)
+ {
+ tmp_desc = socket (p->ai_family, p->ai_socktype, p->ai_protocol);
+
+ if (tmp_desc >= 0)
+ break;
+ }
+
+ if (p == NULL)
+ perror_with_name ("Cannot open socket");
+
+ /* Allow rapid reuse of this port. */
+ tmp = 1;
+ setsockopt (tmp_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
+ sizeof (tmp));
+
+ switch (p->ai_family)
+ {
+ case AF_INET:
+ ((struct sockaddr_in *) p->ai_addr)->sin_addr.s_addr = INADDR_ANY;
+ break;
+ case AF_INET6:
+ ((struct sockaddr_in6 *) p->ai_addr)->sin6_addr = in6addr_any;
+ break;
+ default:
+ fprintf (stderr, "Invalid 'ai_family' %d\n", p->ai_family);
+ exit (1);
+ }
+
+ if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
+ perror_with_name ("Can't bind address");
+
+ if (p->ai_socktype == SOCK_DGRAM)
+ remote_desc = tmp_desc;
+ else
+ {
+ struct sockaddr_storage sockaddr;
+ socklen_t sockaddrsize = sizeof (sockaddr);
+ char orig_host[64], orig_port[16];
+
+ if (listen (tmp_desc, 1) != 0)
+ perror_with_name ("Can't listen on socket");
+
+ remote_desc = accept (tmp_desc, (struct sockaddr *) &sockaddr,
+ &sockaddrsize);
- tmp = sizeof (sockaddr);
- remote_desc = accept (tmp_desc, (struct sockaddr *) &sockaddr, &tmp);
if (remote_desc == -1)
perror_with_name ("Accept failed");
@@ -239,6 +343,16 @@ remote_open (char *name)
setsockopt (remote_desc, IPPROTO_TCP, TCP_NODELAY,
(char *) &tmp, sizeof (tmp));
+ if (getnameinfo ((struct sockaddr *) &sockaddr, sockaddrsize,
+ orig_host, sizeof (orig_host),
+ orig_port, sizeof (orig_port),
+ NI_NUMERICHOST | NI_NUMERICSERV) == 0)
+ {
+ fprintf (stderr, "Remote debugging from host %s, port %s\n",
+ orig_host, orig_port);
+ fflush (stderr);
+ }
+
#ifndef USE_WIN32API
close (tmp_desc); /* No longer need this */
@@ -254,8 +368,9 @@ remote_open (char *name)
fcntl (remote_desc, F_SETFL, FASYNC);
#endif
- fprintf (stderr, "Replay logfile using %s\n", name);
+ fprintf (stderr, "Replay logfile using %s\n", orig_name);
fflush (stderr);
+ free (orig_name);
}
static int
@@ -26,6 +26,8 @@
#include "dll.h"
#include "rsp-low.h"
#include "gdbthread.h"
+#include "netstuff.h"
+#include "filestuff.h"
#include <ctype.h>
#if HAVE_SYS_IOCTL_H
#include <sys/ioctl.h>
@@ -63,6 +65,7 @@
#if USE_WIN32API
#include <winsock2.h>
+#include <wspiapi.h>
#endif
#if __QNX__
@@ -156,19 +159,18 @@ enable_async_notification (int fd)
static int
handle_accept_event (int err, gdb_client_data client_data)
{
- struct sockaddr_in sockaddr;
- socklen_t tmp;
+ struct sockaddr_storage sockaddr;
+ socklen_t len = sizeof (sockaddr);
if (debug_threads)
debug_printf ("handling possible accept event\n");
- tmp = sizeof (sockaddr);
- remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
+ remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
if (remote_desc == -1)
perror_with_name ("Accept failed");
/* Enable TCP keep alive process. */
- tmp = 1;
+ socklen_t tmp = 1;
setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
(char *) &tmp, sizeof (tmp));
@@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
delete_file_handler (listen_desc);
/* Convert IP address to string. */
- fprintf (stderr, "Remote debugging from host %s\n",
- inet_ntoa (sockaddr.sin_addr));
+ char orig_host[64], orig_port[16];
+
+ int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
+ orig_host, sizeof (orig_host),
+ orig_port, sizeof (orig_port),
+ NI_NUMERICHOST | NI_NUMERICSERV);
+
+ if (r != 0)
+ fprintf (stderr, _("Could not obtain remote address: %s\n"),
+ gai_strerror (r));
+ else
+ fprintf (stderr, "Remote debugging from host %s, port %s\n", orig_host,
+ orig_port);
enable_async_notification (remote_desc);
@@ -222,14 +235,10 @@ handle_accept_event (int err, gdb_client_data client_data)
void
remote_prepare (const char *name)
{
- const char *port_str;
#ifdef USE_WIN32API
static int winsock_initialized;
#endif
- int port;
- struct sockaddr_in sockaddr;
socklen_t tmp;
- char *port_end;
remote_is_stdio = 0;
if (strcmp (name, STDIO_CONNECTION_NAME) == 0)
@@ -242,17 +251,25 @@ remote_prepare (const char *name)
return;
}
- port_str = strchr (name, ':');
- if (port_str == NULL)
+ struct addrinfo hint;
+ struct addrinfo *ainfo;
+ std::string host_str, port_str;
+
+ memset (&hint, 0, sizeof (hint));
+ /* Assume no prefix will be passed, therefore we should use
+ AF_UNSPEC. */
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_socktype = SOCK_STREAM;
+ hint.ai_protocol = IPPROTO_TCP;
+
+ parse_hostname_without_prefix (name, host_str, port_str, &hint);
+
+ if (port_str.empty ())
{
transport_is_reliable = 0;
return;
}
- port = strtoul (port_str + 1, &port_end, 10);
- if (port_str[1] == '\0' || *port_end != '\0')
- error ("Bad port argument: %s", name);
-
#ifdef USE_WIN32API
if (!winsock_initialized)
{
@@ -263,8 +280,25 @@ remote_prepare (const char *name)
}
#endif
- listen_desc = socket (PF_INET, SOCK_STREAM, IPPROTO_TCP);
- if (listen_desc == -1)
+ int r = getaddrinfo (host_str.c_str (), port_str.c_str (), &hint, &ainfo);
+
+ if (r != 0)
+ error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
+
+ scoped_free_addrinfo freeaddrinfo (ainfo);
+
+ struct addrinfo *iter;
+
+ for (iter = ainfo; iter != NULL; iter = iter->ai_next)
+ {
+ listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
+ iter->ai_protocol);
+
+ if (listen_desc >= 0)
+ break;
+ }
+
+ if (iter == NULL)
perror_with_name ("Can't open socket");
/* Allow rapid reuse of this port. */
@@ -272,14 +306,25 @@ remote_prepare (const char *name)
setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
sizeof (tmp));
- sockaddr.sin_family = PF_INET;
- sockaddr.sin_port = htons (port);
- sockaddr.sin_addr.s_addr = INADDR_ANY;
+ switch (iter->ai_family)
+ {
+ case AF_INET:
+ ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
+ break;
+ case AF_INET6:
+ ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Invalid 'ai_family' %d\n"), iter->ai_family);
+ }
- if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
- || listen (listen_desc, 1))
+ if (bind (listen_desc, iter->ai_addr, iter->ai_addrlen) != 0)
perror_with_name ("Can't bind address");
+ if (listen (listen_desc, 1) != 0)
+ perror_with_name ("Can't listen on socket");
+
transport_is_reliable = 1;
}
@@ -354,18 +399,24 @@ remote_open (const char *name)
#endif /* USE_WIN32API */
else
{
- int port;
- socklen_t len;
- struct sockaddr_in sockaddr;
-
- len = sizeof (sockaddr);
- if (getsockname (listen_desc,
- (struct sockaddr *) &sockaddr, &len) < 0
- || len < sizeof (sockaddr))
+ char listen_port[16];
+ struct sockaddr_storage sockaddr;
+ socklen_t len = sizeof (sockaddr);
+
+ if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
perror_with_name ("Can't determine port");
- port = ntohs (sockaddr.sin_port);
- fprintf (stderr, "Listening on port %d\n", port);
+ int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
+ NULL, 0,
+ listen_port, sizeof (listen_port),
+ NI_NUMERICSERV);
+
+ if (r != 0)
+ fprintf (stderr, _("Can't obtain port where we are listening: %s"),
+ gai_strerror (r));
+ else
+ fprintf (stderr, "Listening on port %s\n", listen_port);
+
fflush (stderr);
/* Register the event loop handler. */
@@ -25,6 +25,7 @@
#include "cli/cli-decode.h"
#include "cli/cli-setshow.h"
#include "filestuff.h"
+#include "netstuff.h"
#include <sys/types.h>
@@ -39,6 +40,7 @@
#ifdef USE_WIN32API
#include <winsock2.h>
+#include <wspiapi.h>
#ifndef ETIMEDOUT
#define ETIMEDOUT WSAETIMEDOUT
#endif
@@ -158,166 +160,157 @@ wait_for_connect (struct serial *scb, unsigned int *polls)
int
net_open (struct serial *scb, const char *name)
{
- char hostname[100];
- const char *port_str;
- int n, port, tmp;
- int use_udp;
- struct hostent *hostent;
- struct sockaddr_in sockaddr;
+ int n;
+ bool use_udp;
#ifdef USE_WIN32API
u_long ioarg;
#else
int ioarg;
#endif
unsigned int polls = 0;
+ struct addrinfo hint;
+ struct addrinfo *ainfo;
+ std::string host_str, port_str;
- use_udp = 0;
- if (startswith (name, "udp:"))
- {
- use_udp = 1;
- name = name + 4;
- }
- else if (startswith (name, "tcp:"))
- name = name + 4;
-
- port_str = strchr (name, ':');
+ memset (&hint, 0, sizeof (hint));
+ /* Assume no prefix will be passed, therefore we should use
+ AF_UNSPEC. */
+ hint.ai_family = AF_INET;
+ hint.ai_socktype = SOCK_STREAM;
+ hint.ai_protocol = IPPROTO_TCP;
- if (!port_str)
- error (_("net_open: No colon in host name!")); /* Shouldn't ever
- happen. */
+ parse_hostname (name, host_str, port_str, &hint);
- tmp = std::min (port_str - name, (ptrdiff_t) sizeof hostname - 1);
- strncpy (hostname, name, tmp); /* Don't want colon. */
- hostname[tmp] = '\000'; /* Tie off host name. */
- port = atoi (port_str + 1);
+ if (port_str.empty ())
+ error (_("Missing port on hostname '%s'"), name);
- /* Default hostname is localhost. */
- if (!hostname[0])
- strcpy (hostname, "localhost");
+ int r = getaddrinfo (host_str.c_str (), port_str.c_str (), &hint, &ainfo);
- hostent = gethostbyname (hostname);
- if (!hostent)
+ if (r != 0)
{
- fprintf_unfiltered (gdb_stderr, "%s: unknown host\n", hostname);
+ fprintf_unfiltered (gdb_stderr, _("%s: cannot resolve name: %s\n"),
+ name, gai_strerror (r));
errno = ENOENT;
return -1;
}
- sockaddr.sin_family = PF_INET;
- sockaddr.sin_port = htons (port);
- memcpy (&sockaddr.sin_addr.s_addr, hostent->h_addr,
- sizeof (struct in_addr));
+ scoped_free_addrinfo free_ainfo (ainfo);
- retry:
+ struct addrinfo *cur_ainfo;
- if (use_udp)
- scb->fd = gdb_socket_cloexec (PF_INET, SOCK_DGRAM, 0);
- else
- scb->fd = gdb_socket_cloexec (PF_INET, SOCK_STREAM, 0);
+ for (cur_ainfo = ainfo; cur_ainfo != NULL; cur_ainfo = cur_ainfo->ai_next)
+ {
+retry:
+ scb->fd = gdb_socket_cloexec (cur_ainfo->ai_family,
+ cur_ainfo->ai_socktype,
+ cur_ainfo->ai_protocol);
- if (scb->fd == -1)
- return -1;
-
- /* Set socket nonblocking. */
- ioarg = 1;
- ioctl (scb->fd, FIONBIO, &ioarg);
+ if (scb->fd < 0)
+ continue;
- /* Use Non-blocking connect. connect() will return 0 if connected
- already. */
- n = connect (scb->fd, (struct sockaddr *) &sockaddr, sizeof (sockaddr));
+ /* Set socket nonblocking. */
+ ioarg = 1;
+ ioctl (scb->fd, FIONBIO, &ioarg);
- if (n < 0)
- {
+ /* Use Non-blocking connect. connect() will return 0 if connected
+ already. */
+ n = connect (scb->fd, cur_ainfo->ai_addr, cur_ainfo->ai_addrlen);
+
+ if (n < 0)
+ {
#ifdef USE_WIN32API
- int err = WSAGetLastError();
+ int err = WSAGetLastError();
#else
- int err = errno;
+ int err = errno;
#endif
- /* Maybe we're waiting for the remote target to become ready to
- accept connections. */
- if (tcp_auto_retry
+ /* Maybe we're waiting for the remote target to become ready to
+ accept connections. */
+ if (tcp_auto_retry
#ifdef USE_WIN32API
- && err == WSAECONNREFUSED
+ && err == WSAECONNREFUSED
#else
- && err == ECONNREFUSED
+ && err == ECONNREFUSED
#endif
- && wait_for_connect (NULL, &polls) >= 0)
- {
- close (scb->fd);
- goto retry;
- }
+ && wait_for_connect (NULL, &polls) >= 0)
+ {
+ close (scb->fd);
+ goto retry;
+ }
- if (
+ if (
#ifdef USE_WIN32API
- /* Under Windows, calling "connect" with a non-blocking socket
- results in WSAEWOULDBLOCK, not WSAEINPROGRESS. */
- err != WSAEWOULDBLOCK
+ /* Under Windows, calling "connect" with a non-blocking socket
+ results in WSAEWOULDBLOCK, not WSAEINPROGRESS. */
+ err != WSAEWOULDBLOCK
#else
- err != EINPROGRESS
+ err != EINPROGRESS
#endif
- )
- {
- errno = err;
- net_close (scb);
- return -1;
+ )
+ {
+ errno = err;
+ continue;
+ }
+
+ /* Looks like we need to wait for the connect. */
+ do
+ {
+ n = wait_for_connect (scb, &polls);
+ }
+ while (n == 0);
+ if (n < 0)
+ continue;
}
- /* Looks like we need to wait for the connect. */
- do
- {
- n = wait_for_connect (scb, &polls);
- }
- while (n == 0);
- if (n < 0)
- {
- net_close (scb);
- return -1;
- }
- }
-
- /* Got something. Is it an error? */
- {
- int res, err;
- socklen_t len;
-
- len = sizeof (err);
- /* On Windows, the fourth parameter to getsockopt is a "char *";
- on UNIX systems it is generally "void *". The cast to "char *"
- is OK everywhere, since in C++ any data pointer type can be
- implicitly converted to "void *". */
- res = getsockopt (scb->fd, SOL_SOCKET, SO_ERROR, (char *) &err, &len);
- if (res < 0 || err)
+ /* Got something. Is it an error? */
{
- /* Maybe the target still isn't ready to accept the connection. */
- if (tcp_auto_retry
+ int res, err;
+ socklen_t len = sizeof (err);
+
+ /* On Windows, the fourth parameter to getsockopt is a "char *";
+ on UNIX systems it is generally "void *". The cast to "char *"
+ is OK everywhere, since in C++ any data pointer type can be
+ implicitly converted to "void *". */
+ res = getsockopt (scb->fd, SOL_SOCKET, SO_ERROR, (char *) &err, &len);
+ if (res < 0 || err)
+ {
+ /* Maybe the target still isn't ready to accept the connection. */
+ if (tcp_auto_retry
#ifdef USE_WIN32API
- && err == WSAECONNREFUSED
+ && err == WSAECONNREFUSED
#else
- && err == ECONNREFUSED
+ && err == ECONNREFUSED
#endif
- && wait_for_connect (NULL, &polls) >= 0)
- {
- close (scb->fd);
- goto retry;
+ && wait_for_connect (NULL, &polls) >= 0)
+ {
+ close (scb->fd);
+ goto retry;
+ }
+ if (err)
+ errno = err;
+ continue;
}
- if (err)
- errno = err;
- net_close (scb);
- return -1;
}
- }
+ break;
+ }
+
+ if (cur_ainfo == NULL)
+ {
+ net_close (scb);
+ return -1;
+ }
/* Turn off nonblocking. */
ioarg = 0;
ioctl (scb->fd, FIONBIO, &ioarg);
- if (use_udp == 0)
+ if (cur_ainfo->ai_socktype == IPPROTO_TCP)
{
/* Disable Nagle algorithm. Needed in some cases. */
- tmp = 1;
+ int tmp = 1;
+
setsockopt (scb->fd, IPPROTO_TCP, TCP_NODELAY,
- (char *)&tmp, sizeof (tmp));
+ (char *) &tmp, sizeof (tmp));
}
#ifdef SIGPIPE
@@ -259,6 +259,13 @@ This make (not runtest) variable is used to specify whether the
testsuite preloads the read1.so library into expect. Any non-empty
value means true. See "Race detection" below.
+GDB_TEST_IPV6
+
+This variable makes the tests related to GDBserver to run with IPv6
+local addresses, instead of IPv4. This is useful to test the IPv6
+support, and only makes sense for the native-gdbserver and the
+native-extended-gdbserver boards.
+
Race detection
**************
@@ -32,3 +32,8 @@ set_board_info gdb,nofileio 1
set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
set GDBFLAGS "${GDBFLAGS} -ex \"set auto-connect-native-target off\""
+
+# Helper function that returns a local IPv6 address to connect to.
+proc get_comm_port_localhost_ipv6 { port } {
+ return "\\\[::1\\\]:${port}"
+}
@@ -24,7 +24,12 @@ load_generic_config "extended-gdbserver"
load_board_description "gdbserver-base"
load_board_description "local-board"
-set_board_info sockethost "localhost:"
+if { [info exists GDB_TEST_IPV6] } {
+ set_board_info sockethost "tcp6:\[::1\]:"
+ set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
+} else {
+ set_board_info sockethost "localhost:"
+}
# We will be using the extended GDB remote protocol.
set_board_info gdb_protocol "extended-remote"
@@ -30,7 +30,12 @@ set_board_info gdb,do_reload_on_run 1
# There's no support for argument-passing (yet).
set_board_info noargs 1
-set_board_info sockethost "localhost:"
+if { [info exists GDB_TEST_IPV6] } {
+ set_board_info sockethost "tcp6:\[::1\]:"
+ set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
+} else {
+ set_board_info sockethost "localhost:"
+}
set_board_info use_gdb_stub 1
set_board_info exit_is_reliable 1
@@ -53,7 +53,7 @@ save_vars { GDBFLAGS } {
set use_gdb_stub 0
gdb_test "target ${gdbserver_protocol} ${gdbserver_gdbport}" \
- "Remote debugging using $gdbserver_gdbport" \
+ "Remote debugging using [string_to_regexp $gdbserver_gdbport]" \
"connect to gdbserver"
gdb_test "run" \