public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
@ 2018-05-10 19:12 Joel Brobecker
  2018-05-10 21:10 ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2018-05-10 19:12 UTC (permalink / raw)
  To: gdb-patches

Hello,

On Windows, using the "-list-thread-groups --available" GDB/MI command
before an inferior is being debugged:

    % gdb -q -i=mi
    =thread-group-added,id="i1"
    =cmd-param-changed,param="auto-load safe-path",value="/"
    (gdb)
    -list-thread-groups --available
    Segmentation fault

Ooops!

The SEGV happens because the -list-thread-groups --available command
triggers a windows_nat_target::xfer_partial call for a TARGET_OBJECT_OSDATA
object.  Until a program is being debugged, the target_ops layer that
gets the call is the Windows "native" layer. Except for a couple of
specific objects (TARGET_OBJECT_MEMORY and TARGET_OBJECT_LIBRARIES),
this layer's xfer_partial method delegates the xfer of other objects
to the target beneath:

    default:
      return beneath->xfer_partial (object, annex,
                                    readbuf, writebuf, offset, len,
                                    xfered_len);

Unfortunately, there is no "beneath layer" in this case, so
beneath is NULL and dereferencing it leads to the SEGV.

This patch fixes the issue by checking beneath before trying
to delegate the request. But I am wondering whether this is
the right place to fix this issue, or whether we should expect
BENEATH to never be NULL. Ideas?

Also, The testcase I am proposing fails on the -list-thread-groups test
when run on GNU/Linux because, on that platform, the command returns
more output than the expect buffer can handle, resulting in an UNRESOLVED
status. How does one usually handle this? The only why I can think of
is a loop of gdb_test_multiple... Other ideas?

gdb/ChangeLog:

        * windows-nat.c (windows_nat_target::xfer_partial): Return
        TARGET_XFER_E_IO if we need to delegate to the target beneath
        but BENEATH is NULL.

gdb/testsuite/ChangeLog:

        * gdb.mi/mi-list-thread-groups-no-inferior.exp: New testcase.

Thanks!
-- 
Joel

---
 .../gdb.mi/mi-list-thread-groups-no-inferior.exp   | 32 ++++++++++++++++++++++
 gdb/windows-nat.c                                  |  7 +++++
 2 files changed, 39 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp

diff --git a/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp b/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp
new file mode 100644
index 0000000..bff6671
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp
@@ -0,0 +1,32 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# 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/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+mi_gdb_test "-list-thread-groups --available" \
+            ".*^(done|error),.*"
+
+# Verify that GDB is still alive.
+
+mi_gdb_test "-data-evaluate-expression 1" \
+            ".*\\^done,value=\"1\"" \
+            "check GDB is still alive"
+
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 16ebd17..0d3a6cc 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2966,6 +2966,13 @@ windows_nat_target::xfer_partial (enum target_object object,
 					    writebuf, offset, len, xfered_len);
 
     default:
+      if (beneath == NULL)
+	{
+	  /* This can happen when requesting the transfer of unsupported
+	     objects before a program has been started (and therefore
+	     with the current_target having no target beneath).  */
+	  return TARGET_XFER_E_IO;
+	}
       return beneath->xfer_partial (object, annex,
 				    readbuf, writebuf, offset, len,
 				    xfered_len);
-- 
2.1.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-05-10 19:12 [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available" Joel Brobecker
@ 2018-05-10 21:10 ` Simon Marchi
  2018-05-11 17:14   ` Pedro Alves
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2018-05-10 21:10 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

On 2018-05-10 14:58, Joel Brobecker wrote:
> Hello,
> 
> On Windows, using the "-list-thread-groups --available" GDB/MI command
> before an inferior is being debugged:
> 
>     % gdb -q -i=mi
>     =thread-group-added,id="i1"
>     =cmd-param-changed,param="auto-load safe-path",value="/"
>     (gdb)
>     -list-thread-groups --available
>     Segmentation fault
> 
> Ooops!
> 
> The SEGV happens because the -list-thread-groups --available command
> triggers a windows_nat_target::xfer_partial call for a 
> TARGET_OBJECT_OSDATA
> object.  Until a program is being debugged, the target_ops layer that
> gets the call is the Windows "native" layer. Except for a couple of
> specific objects (TARGET_OBJECT_MEMORY and TARGET_OBJECT_LIBRARIES),
> this layer's xfer_partial method delegates the xfer of other objects
> to the target beneath:
> 
>     default:
>       return beneath->xfer_partial (object, annex,
>                                     readbuf, writebuf, offset, len,
>                                     xfered_len);
> 
> Unfortunately, there is no "beneath layer" in this case, so
> beneath is NULL and dereferencing it leads to the SEGV.
> 
> This patch fixes the issue by checking beneath before trying
> to delegate the request. But I am wondering whether this is
> the right place to fix this issue, or whether we should expect
> BENEATH to never be NULL. Ideas?

So when an inferior is started, "-list-thread-groups --available" works 
correctly on Windows?  What is the target beneath then, which provides 
the list of available processes on Windows?

Looking at how it works on Linux, it's the process stratum, 
inf_ptrace_target, that answers this request.  On Windows, shouldn't 
windows_nat_target answer this request?  After all, it's the 
responsibility of windows_nat_target to communicate with the Windows OS 
to debug processes natively on it.

> Also, The testcase I am proposing fails on the -list-thread-groups test
> when run on GNU/Linux because, on that platform, the command returns
> more output than the expect buffer can handle, resulting in an 
> UNRESOLVED
> status. How does one usually handle this? The only why I can think of
> is a loop of gdb_test_multiple... Other ideas?

What's the difference between the new test case and 
gdb.mi/list-thread-groups-available.exp?  In that one too, 
-list-thread-groups --available is executed with no inferior started.  
It also uses mi_gdb_test though, so it probably hits the same 
limitation.

As a quick and dirty hack, is it possible to just increase temporarily 
the size of the buffer to something that will surely be large enough?  
Otherwise, using gdb_test_multiple or maybe gdb_expect to consume the 
output little by little sounds good.

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-05-10 21:10 ` Simon Marchi
@ 2018-05-11 17:14   ` Pedro Alves
  2018-05-16 15:52     ` Simon Marchi
  2018-06-02  0:59     ` Joel Brobecker
  0 siblings, 2 replies; 8+ messages in thread
From: Pedro Alves @ 2018-05-11 17:14 UTC (permalink / raw)
  To: Simon Marchi, Joel Brobecker; +Cc: gdb-patches

On 05/10/2018 10:02 PM, Simon Marchi wrote:

> 
> So when an inferior is started, "-list-thread-groups --available" works correctly on Windows?  What is the target beneath then, which provides the list of available processes on Windows?

After the inferior is started, the Windows target is pushed in the target
stack, so there will be a target beneath, either the exec target, or the
dummy target directly.  Either of those returns TARGET_XFER_E_IO for
this target object.  

The issue here is that before the inferior is started, the
Windows target is not pushed on the target stack.
See target_get_osdata.

> which provides the list of available processes on Windows?

I don't think the feature works at all on Windows.  It's probably
returning an empty list of processes.

> 
> Looking at how it works on Linux, it's the process stratum, inf_ptrace_target, that answers this request.  On Windows, shouldn't windows_nat_target answer this request?  After all, it's the responsibility of windows_nat_target to communicate with the Windows OS to debug processes natively on it.

Yeah, it's just that the Windows port doesn't really implement the
feature at all, AFAICT.  Ideally it'd be implemented.  Otherwise,
I guess handling the case of not having a target beneath
here is reasonable.

> 
>> Also, The testcase I am proposing fails on the -list-thread-groups test
>> when run on GNU/Linux because, on that platform, the command returns
>> more output than the expect buffer can handle, resulting in an UNRESOLVED
>> status. How does one usually handle this? The only why I can think of
>> is a loop of gdb_test_multiple... Other ideas?
> 

> What's the difference between the new test case and gdb.mi/list-thread-groups-available.exp?  In that one too, -list-thread-groups --available is executed with no inferior started.  It also uses mi_gdb_test though, so it probably hits the same limitation.

I've actually saw that testcase fail before because of this issue.  :-)
I probably saw it when I was running many test jobs in parallel (thus many
processes) or something like that.

> 
> As a quick and dirty hack, is it possible to just increase temporarily the size of the buffer to something that will surely be large enough?  Otherwise, using gdb_test_multiple or maybe gdb_expect to consume the output little by little sounds good.

Yeah, the best way to address this is to consume
output in chunks, with exp_continue.  That fixes it for good.
See for example:

commit 11859c310cd6b6fd892337a5ee1d36921e6d08d8
Author:     Andrew Burgess <andrew.burgess@embecosm.com>
AuthorDate: Mon Apr 9 00:18:34 2018 +0100

    gdb/testsuite: Handle targets with lots of registers


I'd prefer that over increasing buffer sizes.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-05-11 17:14   ` Pedro Alves
@ 2018-05-16 15:52     ` Simon Marchi
  2018-05-16 16:44       ` Pedro Alves
  2018-06-02  0:59     ` Joel Brobecker
  1 sibling, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2018-05-16 15:52 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Joel Brobecker, gdb-patches

On 2018-05-11 12:45, Pedro Alves wrote:
> After the inferior is started, the Windows target is pushed in the 
> target
> stack, so there will be a target beneath, either the exec target, or 
> the
> dummy target directly.  Either of those returns TARGET_XFER_E_IO for
> this target object.
> 
> The issue here is that before the inferior is started, the
> Windows target is not pushed on the target stack.
> See target_get_osdata.

Ah ok, so the target is used without being pushed?  I didn't know it was 
possible.

>> which provides the list of available processes on Windows?
> 
> I don't think the feature works at all on Windows.  It's probably
> returning an empty list of processes.

But Joel reported that the test case fails due to the expect buffer 
being full, so the list must not be empty...  we'll need clarifications 
from him :)

>> What's the difference between the new test case and 
>> gdb.mi/list-thread-groups-available.exp?  In that one too, 
>> -list-thread-groups --available is executed with no inferior started.  
>> It also uses mi_gdb_test though, so it probably hits the same 
>> limitation.
> 
> I've actually saw that testcase fail before because of this issue.  :-)
> I probably saw it when I was running many test jobs in parallel (thus 
> many
> processes) or something like that.

It should be fairly easy to reproduce, start a few thousands "sleep 100 
&" processes and then run the test case.

>> As a quick and dirty hack, is it possible to just increase temporarily 
>> the size of the buffer to something that will surely be large enough?  
>> Otherwise, using gdb_test_multiple or maybe gdb_expect to consume the 
>> output little by little sounds good.
> 
> Yeah, the best way to address this is to consume
> output in chunks, with exp_continue.  That fixes it for good.
> See for example:
> 
> commit 11859c310cd6b6fd892337a5ee1d36921e6d08d8
> Author:     Andrew Burgess <andrew.burgess@embecosm.com>
> AuthorDate: Mon Apr 9 00:18:34 2018 +0100
> 
>     gdb/testsuite: Handle targets with lots of registers
> 
> 
> I'd prefer that over increasing buffer sizes.

Of course!

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-05-16 15:52     ` Simon Marchi
@ 2018-05-16 16:44       ` Pedro Alves
  0 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2018-05-16 16:44 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Joel Brobecker, gdb-patches

On 05/16/2018 04:46 PM, Simon Marchi wrote:
> On 2018-05-11 12:45, Pedro Alves wrote:
>> After the inferior is started, the Windows target is pushed in the target
>> stack, so there will be a target beneath, either the exec target, or the
>> dummy target directly.  Either of those returns TARGET_XFER_E_IO for
>> this target object.
>>
>> The issue here is that before the inferior is started, the
>> Windows target is not pushed on the target stack.
>> See target_get_osdata.
> 
> Ah ok, so the target is used without being pushed?  I didn't know it was possible.

Yeah.  A few methods can be called like that.  target_ops::create_inferior
for instance, is called before the target is pushed, and the implementation
of that method is responsible for pushing itself on the stack on success.

> 
>>> which provides the list of available processes on Windows?
>>
>> I don't think the feature works at all on Windows.  It's probably
>> returning an empty list of processes.
> 
> But Joel reported that the test case fails due to the expect buffer being full, so the list must not be empty...  we'll need clarifications from him :)

The reported failure was on GNU/Linux, not Windows.  :-)

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-05-11 17:14   ` Pedro Alves
  2018-05-16 15:52     ` Simon Marchi
@ 2018-06-02  0:59     ` Joel Brobecker
  2018-06-02 10:26       ` Pedro Alves
  1 sibling, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2018-06-02  0:59 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Simon Marchi, gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

> > Looking at how it works on Linux, it's the process stratum,
> > inf_ptrace_target, that answers this request.  On Windows, shouldn't
> > windows_nat_target answer this request?  After all, it's the
> > responsibility of windows_nat_target to communicate with the Windows
> > OS to debug processes natively on it.
> 
> Yeah, it's just that the Windows port doesn't really implement the
> feature at all, AFAICT.  Ideally it'd be implemented.  Otherwise,
> I guess handling the case of not having a target beneath
> here is reasonable.
> > 
> >> Also, The testcase I am proposing fails on the -list-thread-groups test
> >> when run on GNU/Linux because, on that platform, the command returns
> >> more output than the expect buffer can handle, resulting in an UNRESOLVED
> >> status. How does one usually handle this? The only why I can think of
> >> is a loop of gdb_test_multiple... Other ideas?
[...]
> Yeah, the best way to address this is to consume
> output in chunks, with exp_continue.  That fixes it for good.
> See for example:
> 
> commit 11859c310cd6b6fd892337a5ee1d36921e6d08d8

Thanks for the pointer. I've taken the liberty of updating our
testcase cookbook to reference it as well, so we know where to
look next time we hit that issue.

Attached is an updated version which follows the same principle.

gdb/ChangeLog:

        * windows-nat.c (windows_nat_target::xfer_partial): Return
        TARGET_XFER_E_IO if we need to delegate to the target beneath
        but BENEATH is NULL.

gdb/testsuite/ChangeLog:

        * gdb.mi/mi-list-thread-groups-no-inferior.exp: New testcase.

Test verified on x86_64-linux to confirm that the large amount of
output is properly handled.

OK to push?
-- 
Joel

[-- Attachment #2: 0001-windows-GDB-MI-crash-when-using-list-thread-groups-a.patch --]
[-- Type: text/x-diff, Size: 4182 bytes --]

From 6cf99a70c551b2a672d7732442f2da13154cab0c Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 1 Jun 2018 19:54:38 -0500
Subject: [PATCH] (windows) GDB/MI crash when using "-list-thread-groups
 --available"

On Windows, using the "-list-thread-groups --available" GDB/MI command
before an inferior is being debugged:

    % gdb -q -i=mi
    =thread-group-added,id="i1"
    =cmd-param-changed,param="auto-load safe-path",value="/"
    (gdb)
    -list-thread-groups --available
    Segmentation fault

Ooops!

The SEGV happens because the -list-thread-groups --available command
triggers a windows_nat_target::xfer_partial call for a TARGET_OBJECT_OSDATA
object.  Until a program is being debugged, the target_ops layer that
gets the call is the Windows "native" layer. Except for a couple of
specific objects (TARGET_OBJECT_MEMORY and TARGET_OBJECT_LIBRARIES),
this layer's xfer_partial method delegates the xfer of other objects
to the target beneath:

    default:
      return beneath->xfer_partial (object, annex,
                                    readbuf, writebuf, offset, len,
                                    xfered_len);

Unfortunately, there is no "beneath layer" in this case, so
beneath is NULL and dereferencing it leads to the SEGV.

This patch fixes the issue by checking beneath before trying
to delegate the request.

gdb/ChangeLog:

        * windows-nat.c (windows_nat_target::xfer_partial): Return
        TARGET_XFER_E_IO if we need to delegate to the target beneath
        but BENEATH is NULL.

gdb/testsuite/ChangeLog:

        * gdb.mi/mi-list-thread-groups-no-inferior.exp: New testcase.
---
 .../gdb.mi/mi-list-thread-groups-no-inferior.exp   | 43 ++++++++++++++++++++++
 gdb/windows-nat.c                                  |  7 ++++
 2 files changed, 50 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp

diff --git a/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp b/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp
new file mode 100644
index 0000000..381f6f1
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-list-thread-groups-no-inferior.exp
@@ -0,0 +1,43 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# 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/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+# Try the "-list-thread-groups --available".  This command can generate
+# a very large amount of output, potentially exceeding expect's buffer
+# size.  So we consume the output in chunks.
+
+set test "-list-thread-groups --available"
+gdb_test_multiple $test $test {
+    -re ".*\}" {
+        exp_continue
+    }
+    -re "\r\n$gdb_prompt " {
+        pass $test
+    }
+}
+
+# Verify that GDB is still alive.
+
+mi_gdb_test "-data-evaluate-expression 1" \
+            ".*\\^done,value=\"1\"" \
+            "check GDB is still alive"
+
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 0f24257..e3e36cd 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2966,6 +2966,13 @@ windows_nat_target::xfer_partial (enum target_object object,
 					    writebuf, offset, len, xfered_len);
 
     default:
+      if (beneath == NULL)
+	{
+	  /* This can happen when requesting the transfer of unsupported
+	     objects before a program has been started (and therefore
+	     with the current_target having no target beneath).  */
+	  return TARGET_XFER_E_IO;
+	}
       return beneath->xfer_partial (object, annex,
 				    readbuf, writebuf, offset, len,
 				    xfered_len);
-- 
2.1.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-06-02  0:59     ` Joel Brobecker
@ 2018-06-02 10:26       ` Pedro Alves
  2018-06-04 20:11         ` Joel Brobecker
  0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2018-06-02 10:26 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Simon Marchi, gdb-patches

On 06/02/2018 01:59 AM, Joel Brobecker wrote:

> Thanks for the pointer. I've taken the liberty of updating our
> testcase cookbook to reference it as well, so we know where to
> look next time we hit that issue.

Good idea, thanks.

> 
> Attached is an updated version which follows the same principle.
> 
> gdb/ChangeLog:
> 
>         * windows-nat.c (windows_nat_target::xfer_partial): Return
>         TARGET_XFER_E_IO if we need to delegate to the target beneath
>         but BENEATH is NULL.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.mi/mi-list-thread-groups-no-inferior.exp: New testcase.
> 
> Test verified on x86_64-linux to confirm that the large amount of
> output is properly handled.
> 
> OK to push?

It looks good to me.  A few comments on minor details follow.

How about naming the testcase list-thread-groups-no-inferior.exp
(no "mi-") so that it sits side-by-side with
list-thread-groups-available.exp?

We're missing an intro comment in the .exp file mentioning what the
testcase is about, otherwise it's not clear why we have two separate
testcases for seemingly the same thing.

A couple microscopic nits more:

> +gdb_test_multiple $test $test {
> +    -re ".*\}" {

Leading ".*" is not necessary, it's implied.

> +mi_gdb_test "-data-evaluate-expression 1" \
> +            ".*\\^done,value=\"1\"" \
> +            "check GDB is still alive"
> +

"check" and "test" in test messages is one of my pet peeves.
OK, I'm really exaggerating.  :-)  The point is that all
tests are checking something, making it redundant.
"GDB is still alive" or even "GDB is alive" says the same.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"
  2018-06-02 10:26       ` Pedro Alves
@ 2018-06-04 20:11         ` Joel Brobecker
  0 siblings, 0 replies; 8+ messages in thread
From: Joel Brobecker @ 2018-06-04 20:11 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

Hi Pedro,

> It looks good to me.  A few comments on minor details follow.
> 
> How about naming the testcase list-thread-groups-no-inferior.exp
> (no "mi-") so that it sits side-by-side with
> list-thread-groups-available.exp?

Good idea. Done.

> We're missing an intro comment in the .exp file mentioning what the
> testcase is about, otherwise it's not clear why we have two separate
> testcases for seemingly the same thing.

Also added. Let me know if you think it's not enough information,
and I will add more. That's all I could think of from my perspective,
though.

> A couple microscopic nits more:
> 
> > +gdb_test_multiple $test $test {
> > +    -re ".*\}" {
> 
> Leading ".*" is not necessary, it's implied.

Thanks - fixed!

> 
> > +mi_gdb_test "-data-evaluate-expression 1" \
> > +            ".*\\^done,value=\"1\"" \
> > +            "check GDB is still alive"
> > +
> 
> "check" and "test" in test messages is one of my pet peeves.
> OK, I'm really exaggerating.  :-)  The point is that all
> tests are checking something, making it redundant.
> "GDB is still alive" or even "GDB is alive" says the same.

True, and I like it when it's shorter-with-no-loss. Fixed as well.

Attached is the version I just pushed. I'll make further adjustments
if you see something I missed or the description isn't complete for you.

Thanks again!
-- 
Joel

[-- Attachment #2: 0001-windows-GDB-MI-crash-when-using-list-thread-groups-a.patch --]
[-- Type: text/x-diff, Size: 5347 bytes --]

From 178d6a638693d1a7a66b0553f16b1df95b4f27ee Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 4 Jun 2018 15:03:32 -0500
Subject: [PATCH] (windows) GDB/MI crash when using "-list-thread-groups
 --available"

On Windows, using the "-list-thread-groups --available" GDB/MI command
before an inferior is being debugged:

    % gdb -q -i=mi
    =thread-group-added,id="i1"
    =cmd-param-changed,param="auto-load safe-path",value="/"
    (gdb)
    -list-thread-groups --available
    Segmentation fault

Ooops!

The SEGV happens because the -list-thread-groups --available command
triggers a windows_nat_target::xfer_partial call for a TARGET_OBJECT_OSDATA
object.  Until a program is being debugged, the target_ops layer that
gets the call is the Windows "native" layer. Except for a couple of
specific objects (TARGET_OBJECT_MEMORY and TARGET_OBJECT_LIBRARIES),
this layer's xfer_partial method delegates the xfer of other objects
to the target beneath:

    default:
      return beneath->xfer_partial (object, annex,
                                    readbuf, writebuf, offset, len,
                                    xfered_len);

Unfortunately, there is no "beneath layer" in this case, so
beneath is NULL and dereferencing it leads to the SEGV.

This patch fixes the issue by checking beneath before trying
to delegate the request.

gdb/ChangeLog:

        * windows-nat.c (windows_nat_target::xfer_partial): Return
        TARGET_XFER_E_IO if we need to delegate to the target beneath
        but BENEATH is NULL.

gdb/testsuite/ChangeLog:

        * gdb.mi/list-thread-groups-no-inferior.exp: New testcase.
---
 gdb/ChangeLog                                      |  6 +++
 gdb/testsuite/ChangeLog                            |  4 ++
 .../gdb.mi/list-thread-groups-no-inferior.exp      | 47 ++++++++++++++++++++++
 gdb/windows-nat.c                                  |  7 ++++
 4 files changed, 64 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/list-thread-groups-no-inferior.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d0c43027d6..b5580c429a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-04  Joel Brobecker  <brobecker@adacore.com>
+
+	* windows-nat.c (windows_nat_target::xfer_partial): Return
+	TARGET_XFER_E_IO if we need to delegate to the target beneath
+	but BENEATH is NULL.
+
 2018-06-04  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* Makefile.in (config.status): Add configure.nat as a
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8cef172a26..0e98d7e115 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-06-04  Joel Brobecker  <brobecker@adacore.com>
+
+	* gdb.mi/list-thread-groups-no-inferior.exp: New testcase.
+
 2018-06-01  Joel Brobecker  <brobecker@adacore.com>
 
 	* gdb.ada/bp_fun_addr: New testcase.
diff --git a/gdb/testsuite/gdb.mi/list-thread-groups-no-inferior.exp b/gdb/testsuite/gdb.mi/list-thread-groups-no-inferior.exp
new file mode 100644
index 0000000000..9ab38380c0
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/list-thread-groups-no-inferior.exp
@@ -0,0 +1,47 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# 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/>.
+
+# The purpose of this test is to verify that GDB is able to handle
+# the "-list-thread-groups --available" command, even when there is
+# no inferior. In particular, we want to verify that GDB does not
+# crash.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+# Try the "-list-thread-groups --available".  This command can generate
+# a very large amount of output, potentially exceeding expect's buffer
+# size.  So we consume the output in chunks.
+
+set test "-list-thread-groups --available"
+gdb_test_multiple $test $test {
+    -re "\}" {
+        exp_continue
+    }
+    -re "\r\n$gdb_prompt " {
+        pass $test
+    }
+}
+
+# Verify that GDB is still alive.
+
+mi_gdb_test "-data-evaluate-expression 1" \
+            ".*\\^done,value=\"1\"" \
+            "GDB is still alive"
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 0f24257935..e3e36cdc3e 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2966,6 +2966,13 @@ windows_nat_target::xfer_partial (enum target_object object,
 					    writebuf, offset, len, xfered_len);
 
     default:
+      if (beneath == NULL)
+	{
+	  /* This can happen when requesting the transfer of unsupported
+	     objects before a program has been started (and therefore
+	     with the current_target having no target beneath).  */
+	  return TARGET_XFER_E_IO;
+	}
       return beneath->xfer_partial (object, annex,
 				    readbuf, writebuf, offset, len,
 				    xfered_len);
-- 
2.11.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-04 20:11 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10 19:12 [RFC] (windows) GDB/MI crash when using "-list-thread-groups --available" Joel Brobecker
2018-05-10 21:10 ` Simon Marchi
2018-05-11 17:14   ` Pedro Alves
2018-05-16 15:52     ` Simon Marchi
2018-05-16 16:44       ` Pedro Alves
2018-06-02  0:59     ` Joel Brobecker
2018-06-02 10:26       ` Pedro Alves
2018-06-04 20:11         ` Joel Brobecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).