diff mbox

[RFC] (windows) GDB/MI crash when using "-list-thread-groups --available"

Message ID 20180602005941.f4l5nudlsl3xez4v@adacore.com
State New, archived
Headers

Commit Message

Joel Brobecker June 2, 2018, 12:59 a.m. UTC 
  > > 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?

Comments

Pedro Alves June 2, 2018, 10:26 a.m. UTC | #1 
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
diff mbox

Patch

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