[PATCH] gdb: run 'maint selftest' with an executable loaded

[PATCH] gdb: run 'maint selftest' with an executable loaded

[Andrew Burgess]

I spotted that 'maint selftest' with an executable loaded into GDB,
would (when GDB was compiled for all targets) crash GDB.  I fixed this
with a commit to bfd:

  commit 427e4066afd13d1bf52c849849475f536e285d66
  Date:   Thu May 20 09:16:41 2021 +0100

      gdb/bfd: avoid crash when architecture is forced to csky or riscv

However, this issue was not spotted as we currently only run 'maint
selftest' without an executable loaded.

This commit extends the testsuite to run 'maint selftest' both with
and without an executable loaded into GDB.

Currently, when no executable is loaded into GDB all of the selftest
pass (i.e. the fail count is 0), however, when running with an
executable loaded, I am seeing 1 failure (on an x86-64 GNU/Linux
host).

This failure is from the ARM disassembler tests, it appears that the
disassembler somehow gets itself into a state where it thinks it is in
thumb mode; when running the same test without an executable loaded
this doesn't happen.

This commit doesn't fix the ARM disassembler issue, but I thought it
was worth adding this anyway, as this will spot if GDB again starts to
crash when 'maint selftest' is run.

gdb/testsuite/ChangeLog:

	* gdb.gdb/unittest.c: New file.
	* gdb.gdb/unittest.exp: Run with and without a binary file loaded
	into GDB.
---
 gdb/testsuite/ChangeLog            |  6 +++
 gdb/testsuite/gdb.gdb/unittest.c   | 22 ++++++++++
 gdb/testsuite/gdb.gdb/unittest.exp | 70 ++++++++++++++++++++++--------
 3 files changed, 79 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.gdb/unittest.c

diff --git a/gdb/testsuite/gdb.gdb/unittest.c b/gdb/testsuite/gdb.gdb/unittest.c
new file mode 100644
index 00000000000..9811b15f06d
--- /dev/null
+++ b/gdb/testsuite/gdb.gdb/unittest.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.gdb/unittest.exp b/gdb/testsuite/gdb.gdb/unittest.exp
index 8718999dd33..ed2d4bc0035 100644
--- a/gdb/testsuite/gdb.gdb/unittest.exp
+++ b/gdb/testsuite/gdb.gdb/unittest.exp
@@ -22,25 +22,57 @@ if [gdb_debug_enabled] {
 
 set do_xml_test [expr ![gdb_skip_xml_test]]
 
-gdb_start
-
-set test "maintenance selftest"
-gdb_test_multiple $test $test {
-  -re ".*Running selftest \[^\n\r\]+\." {
-	# The selftests can take some time to complete.  To prevent
-	# timeout spot the 'Running ...' lines going past, so long as
-	# these are produced quickly enough then the overall test will
-	# not timeout.
-	exp_continue
-  }
-  -re "Ran ($decimal) unit tests, 0 failed\r\n$gdb_prompt $" {
-	set num_ran $expect_out(1,string)
-	gdb_assert "$num_ran > 0" $test
-  }
-
-  -re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
-	unsupported $test
-  }
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+proc run_selftests { binfile } {
+    global decimal gdb_prompt
+
+    if { $binfile == "" } {
+	gdb_exit
+	gdb_start
+    } else {
+	clean_restart ${binfile}
+    }
+
+    set test "maintenance selftest"
+    gdb_test_multiple $test $test {
+	-re ".*Running selftest \[^\n\r\]+\." {
+	    # The selftests can take some time to complete.  To prevent
+	    # timeout spot the 'Running ...' lines going past, so long as
+	    # these are produced quickly enough then the overall test will
+	    # not timeout.
+	    exp_continue
+	}
+	-re "Ran ($decimal) unit tests, ($decimal) failed\r\n$gdb_prompt $" {
+	    set num_ran $expect_out(1,string)
+	    set num_failed $expect_out(2,string)
+	    gdb_assert "$num_ran > 0" "$test, ran some tests"
+	    if { $binfile == "" } {
+		gdb_assert "$num_failed == 0" "$test, failed none"
+	    } else {
+		# Ideally we'd have zero fails for this path too, but
+		# when GDB is compiled for all targets, on an x86-64
+		# GNU/Linux host, there is a failure from the ARM
+		# disassembler.
+		gdb_assert "$num_failed <= 1" "$test, failed no more than one"
+	    }
+	}
+	-re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
+	    unsupported $test
+	}
+    }
+}
+
+with_test_prefix "no executable loaded" {
+    run_selftests ""
+}
+
+with_test_prefix "executable loaded" {
+    run_selftests ${binfile}
 }
 
 if { ![is_remote host] && $do_xml_test } {
-- 
2.25.4

[PATCHv2] gdb: run 'maint selftest' with an executable loaded

[Andrew Burgess]

Doh!  The v1 patch was an old version of this change.

Thanks,
Andrew

---

I spotted that 'maint selftest' with an executable loaded into GDB,
would (when GDB was compiled for all targets) crash GDB.  I fixed this
with a commit to bfd:

  commit 427e4066afd13d1bf52c849849475f536e285d66
  Date:   Thu May 20 09:16:41 2021 +0100

      gdb/bfd: avoid crash when architecture is forced to csky or riscv

However, this issue was not spotted as we currently only run 'maint
selftest' without an executable loaded.

This commit extends the testsuite to run 'maint selftest' both with
and without an executable loaded into GDB.

Currently, when no executable is loaded into GDB all of the selftest
pass (i.e. the fail count is 0), however, when running with an
executable loaded, I am seeing 1 failure (on an x86-64 GNU/Linux
host).

This failure is from the ARM disassembler tests, it appears that the
disassembler somehow gets itself into a state where it thinks it is in
thumb mode; when running the same test without an executable loaded
this doesn't happen.

This commit doesn't fix the ARM disassembler issue, but I thought it
was worth adding this anyway, as this will spot if GDB again starts to
crash when 'maint selftest' is run.

gdb/testsuite/ChangeLog:

	* gdb.gdb/unittest.c: New file.
	* gdb.gdb/unittest.exp: Run with and without a binary file loaded
	into GDB.
---
 gdb/testsuite/ChangeLog            |  6 +++
 gdb/testsuite/gdb.gdb/unittest.c   | 22 ++++++++++
 gdb/testsuite/gdb.gdb/unittest.exp | 69 ++++++++++++++++++++++--------
 3 files changed, 78 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.gdb/unittest.c

diff --git a/gdb/testsuite/gdb.gdb/unittest.c b/gdb/testsuite/gdb.gdb/unittest.c
new file mode 100644
index 00000000000..9811b15f06d
--- /dev/null
+++ b/gdb/testsuite/gdb.gdb/unittest.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.gdb/unittest.exp b/gdb/testsuite/gdb.gdb/unittest.exp
index 8718999dd33..61a6c0efb4b 100644
--- a/gdb/testsuite/gdb.gdb/unittest.exp
+++ b/gdb/testsuite/gdb.gdb/unittest.exp
@@ -22,25 +22,56 @@ if [gdb_debug_enabled] {
 
 set do_xml_test [expr ![gdb_skip_xml_test]]
 
-gdb_start
-
-set test "maintenance selftest"
-gdb_test_multiple $test $test {
-  -re ".*Running selftest \[^\n\r\]+\." {
-	# The selftests can take some time to complete.  To prevent
-	# timeout spot the 'Running ...' lines going past, so long as
-	# these are produced quickly enough then the overall test will
-	# not timeout.
-	exp_continue
-  }
-  -re "Ran ($decimal) unit tests, 0 failed\r\n$gdb_prompt $" {
-	set num_ran $expect_out(1,string)
-	gdb_assert "$num_ran > 0" $test
-  }
-
-  -re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
-	unsupported $test
-  }
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+proc run_selftests { binfile } {
+    global decimal gdb_prompt
+
+    if { $binfile == "" } {
+	gdb_exit
+	gdb_start
+    } else {
+	clean_restart ${binfile}
+    }
+
+    set test "maintenance selftest"
+    gdb_test_multiple $test $test {
+	-re ".*Running selftest \[^\n\r\]+\." {
+	    # The selftests can take some time to complete.  To prevent
+	    # timeout spot the 'Running ...' lines going past, so long as
+	    # these are produced quickly enough then the overall test will
+	    # not timeout.
+	    exp_continue
+	}
+	-re "Ran ($decimal) unit tests, ($decimal) failed\r\n$gdb_prompt $" {
+	    set num_ran $expect_out(1,string)
+	    set num_failed $expect_out(2,string)
+	    gdb_assert "$num_ran > 0" "$test, ran some tests"
+
+	    if { $binfile != "" } {
+		# There's a known issue here (see PR gdb/27891),
+		# however, we should not have more than 1 failure.
+		gdb_assert "$num_failed <= 1" "$test, failed no more than 1"
+		setup_kfail "gdb/27891" "*-*-*"
+	    }
+	    gdb_assert "$num_failed == 0" "$test, failed none"
+	}
+	-re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
+	    unsupported $test
+	}
+    }
+}
+
+with_test_prefix "no executable loaded" {
+    run_selftests ""
+}
+
+with_test_prefix "executable loaded" {
+    run_selftests ${binfile}
 }
 
 if { ![is_remote host] && $do_xml_test } {
-- 
2.25.4

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Luis Machado]

On 5/20/21 10:29 AM, Andrew Burgess wrote:
> I spotted that 'maint selftest' with an executable loaded into GDB,
> would (when GDB was compiled for all targets) crash GDB.  I fixed this
> with a commit to bfd:

I don't understand the purpose of being able to run selftests with an 
executable loaded, or, more generally, running selftests when any GDB 
state is capable of interfering with the tests.

Should we force the selftests to cleanup the state before we run them? 
Or not allow running selftests when there is a debugging session already 
stablished?

> 
>    commit 427e4066afd13d1bf52c849849475f536e285d66
>    Date:   Thu May 20 09:16:41 2021 +0100
> 
>        gdb/bfd: avoid crash when architecture is forced to csky or riscv
> 
> However, this issue was not spotted as we currently only run 'maint
> selftest' without an executable loaded.
> 
> This commit extends the testsuite to run 'maint selftest' both with
> and without an executable loaded into GDB.
> 
> Currently, when no executable is loaded into GDB all of the selftest
> pass (i.e. the fail count is 0), however, when running with an
> executable loaded, I am seeing 1 failure (on an x86-64 GNU/Linux
> host).
> 
> This failure is from the ARM disassembler tests, it appears that the
> disassembler somehow gets itself into a state where it thinks it is in
> thumb mode; when running the same test without an executable loaded
> this doesn't happen.

Do you have more information about that?

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Andrew Burgess]

* Luis Machado <luis.machado@linaro.org> [2021-05-20 11:29:07 -0300]:

> On 5/20/21 10:29 AM, Andrew Burgess wrote:
> > I spotted that 'maint selftest' with an executable loaded into GDB,
> > would (when GDB was compiled for all targets) crash GDB.  I fixed this
> > with a commit to bfd:
> 
> I don't understand the purpose of being able to run selftests with an
> executable loaded, or, more generally, running selftests when any GDB state
> is capable of interfering with the tests.
> 
> Should we force the selftests to cleanup the state before we run them? Or
> not allow running selftests when there is a debugging session already
> stablished?

I'm not trying to claim such a thing is useful, but I do feel pretty
strongly that running 'maint selftest' (with an executable loaded)
shouldn't cause GDB to segfault.  To ensure it doesn't get broken
again in the future I'd suggest we need add this configuration to the
testsuite.

I guess one possible solution would have been to add a check within
the 'maint selftest' command that throws an error if an executable is
loaded, but, that would have meant we missed a real bug (I claim) in
bfd.

You'll need to check out commit 427e4066afd1 for details of the issue
I found, but it could be reproduced (though I admit via a rather weird
set of steps) outside of 'maint selftest'.

I understand your concerns about the selftests potentially depending
on GDB being in a vanilla state, which might be corrupted by loading
an executable, right now this doesn't seem to be the case (too much, I
guess the ARM failure is an example of this happening).

If in the future we get more such cases, I'd have no problems with us
increasing the number of acceptable selftest failures.

> 
> > 
> >    commit 427e4066afd13d1bf52c849849475f536e285d66
> >    Date:   Thu May 20 09:16:41 2021 +0100
> > 
> >        gdb/bfd: avoid crash when architecture is forced to csky or riscv
> > 
> > However, this issue was not spotted as we currently only run 'maint
> > selftest' without an executable loaded.
> > 
> > This commit extends the testsuite to run 'maint selftest' both with
> > and without an executable loaded into GDB.
> > 
> > Currently, when no executable is loaded into GDB all of the selftest
> > pass (i.e. the fail count is 0), however, when running with an
> > executable loaded, I am seeing 1 failure (on an x86-64 GNU/Linux
> > host).
> > 
> > This failure is from the ARM disassembler tests, it appears that the
> > disassembler somehow gets itself into a state where it thinks it is in
> > thumb mode; when running the same test without an executable loaded
> > this doesn't happen.
> 
> Do you have more information about that?

Not really, the problem is the 'force_thumb' static global in
opcode/arm-dis.c.  When running without an executable loaded this
variable remains 'false'.  When run with an executable loaded I'm
seeing this get set 'true' during an earlier test, then when we get to
the print_one_insn tests we end up disassembling in thumb mode.

At this point I stopped looking, for exactly the reasons you raised
above.

Thanks,
Andrew

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Luis Machado]

On 5/20/21 12:41 PM, Andrew Burgess wrote:
> * Luis Machado <luis.machado@linaro.org> [2021-05-20 11:29:07 -0300]:
> 
>> On 5/20/21 10:29 AM, Andrew Burgess wrote:
>>> I spotted that 'maint selftest' with an executable loaded into GDB,
>>> would (when GDB was compiled for all targets) crash GDB.  I fixed this
>>> with a commit to bfd:
>>
>> I don't understand the purpose of being able to run selftests with an
>> executable loaded, or, more generally, running selftests when any GDB state
>> is capable of interfering with the tests.
>>
>> Should we force the selftests to cleanup the state before we run them? Or
>> not allow running selftests when there is a debugging session already
>> stablished?
> 
> I'm not trying to claim such a thing is useful, but I do feel pretty
> strongly that running 'maint selftest' (with an executable loaded)
> shouldn't cause GDB to segfault.  To ensure it doesn't get broken
> again in the future I'd suggest we need add this configuration to the
> testsuite.
> 
> I guess one possible solution would have been to add a check within
> the 'maint selftest' command that throws an error if an executable is
> loaded, but, that would have meant we missed a real bug (I claim) in
> bfd.
> 
> You'll need to check out commit 427e4066afd1 for details of the issue
> I found, but it could be reproduced (though I admit via a rather weird
> set of steps) outside of 'maint selftest'.
> 
> I understand your concerns about the selftests potentially depending
> on GDB being in a vanilla state, which might be corrupted by loading
> an executable, right now this doesn't seem to be the case (too much, I
> guess the ARM failure is an example of this happening).
> 
> If in the future we get more such cases, I'd have no problems with us
> increasing the number of acceptable selftest failures.

Right. I'm not against the fix, but this shows the selftest design in 
GDB is kinda broken in this regard. It is not self-contained and seems 
to rely on external state.

Just wanted to point out that we shouldn't need to handle running the 
selftests with an executable loaded. It makes me wonder what other 
options we can tweak when GDB is running that will cause the selftests 
to hit some unpredictable situations.

> 
>>
>>>
>>>     commit 427e4066afd13d1bf52c849849475f536e285d66
>>>     Date:   Thu May 20 09:16:41 2021 +0100
>>>
>>>         gdb/bfd: avoid crash when architecture is forced to csky or riscv
>>>
>>> However, this issue was not spotted as we currently only run 'maint
>>> selftest' without an executable loaded.
>>>
>>> This commit extends the testsuite to run 'maint selftest' both with
>>> and without an executable loaded into GDB.
>>>
>>> Currently, when no executable is loaded into GDB all of the selftest
>>> pass (i.e. the fail count is 0), however, when running with an
>>> executable loaded, I am seeing 1 failure (on an x86-64 GNU/Linux
>>> host).
>>>
>>> This failure is from the ARM disassembler tests, it appears that the
>>> disassembler somehow gets itself into a state where it thinks it is in
>>> thumb mode; when running the same test without an executable loaded
>>> this doesn't happen.
>>
>> Do you have more information about that?
> 
> Not really, the problem is the 'force_thumb' static global in
> opcode/arm-dis.c.  When running without an executable loaded this
> variable remains 'false'.  When run with an executable loaded I'm
> seeing this get set 'true' during an earlier test, then when we get to
> the print_one_insn tests we end up disassembling in thumb mode.
> 
> At this point I stopped looking, for exactly the reasons you raised
> above.

Thanks, that's useful to know.

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Andrew Burgess]

* Luis Machado <luis.machado@linaro.org> [2021-05-20 13:06:04 -0300]:

> On 5/20/21 12:41 PM, Andrew Burgess wrote:
> > * Luis Machado <luis.machado@linaro.org> [2021-05-20 11:29:07 -0300]:
> > 
> > > On 5/20/21 10:29 AM, Andrew Burgess wrote:
> > > > I spotted that 'maint selftest' with an executable loaded into GDB,
> > > > would (when GDB was compiled for all targets) crash GDB.  I fixed this
> > > > with a commit to bfd:
> > > 
> > > I don't understand the purpose of being able to run selftests with an
> > > executable loaded, or, more generally, running selftests when any GDB state
> > > is capable of interfering with the tests.
> > > 
> > > Should we force the selftests to cleanup the state before we run them? Or
> > > not allow running selftests when there is a debugging session already
> > > stablished?
> > 
> > I'm not trying to claim such a thing is useful, but I do feel pretty
> > strongly that running 'maint selftest' (with an executable loaded)
> > shouldn't cause GDB to segfault.  To ensure it doesn't get broken
> > again in the future I'd suggest we need add this configuration to the
> > testsuite.
> > 
> > I guess one possible solution would have been to add a check within
> > the 'maint selftest' command that throws an error if an executable is
> > loaded, but, that would have meant we missed a real bug (I claim) in
> > bfd.
> > 
> > You'll need to check out commit 427e4066afd1 for details of the issue
> > I found, but it could be reproduced (though I admit via a rather weird
> > set of steps) outside of 'maint selftest'.
> > 
> > I understand your concerns about the selftests potentially depending
> > on GDB being in a vanilla state, which might be corrupted by loading
> > an executable, right now this doesn't seem to be the case (too much, I
> > guess the ARM failure is an example of this happening).
> > 
> > If in the future we get more such cases, I'd have no problems with us
> > increasing the number of acceptable selftest failures.
> 
> Right. I'm not against the fix, but this shows the selftest design in GDB is
> kinda broken in this regard. It is not self-contained and seems to rely on
> external state.
> 
> Just wanted to point out that we shouldn't need to handle running the
> selftests with an executable loaded. It makes me wonder what other options
> we can tweak when GDB is running that will cause the selftests to hit some
> unpredictable situations.

You'll have to excuse me for being a little slow today.  I believe I
understand your concerns, which I think are:

  Selftest was not designed to be run in any situation other than in a
  vanilla GDB, with no executable loaded, so it's not surprising that
  it broke when we did "other" things.

What I'm struggling with is to translate your concerns into what
action you think I should take.

I think I see a couple of possibilities:

  1. Continue with the test I suggest (running selftest with an
  executable loaded), but we acknowledge that we might have to accept
  some selftest failures in this case, or

  2. Modify 'maint selftest' so it throws an error if GDB has an
  executable loaded, write a new test that checks this error occurs,
  and, write an additional test that reveals the bug I originally saw
  (via 'maint selftest') but through the alternative route.

  3. Maybe you have a different suggestion?

Honestly, I'm prepare to go with any approach, it's just, I found a
way to crash GDB, I fixed it, and I think we should make sure the same
failure can't be introduced again in the future.

Thanks for all your input,

Andrew

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Simon Marchi]

> You'll have to excuse me for being a little slow today.  I believe I
> understand your concerns, which I think are:
> 
>   Selftest was not designed to be run in any situation other than in a
>   vanilla GDB, with no executable loaded, so it's not surprising that
>   it broke when we did "other" things.
> 
> What I'm struggling with is to translate your concerns into what
> action you think I should take.
> 
> I think I see a couple of possibilities:
> 
>   1. Continue with the test I suggest (running selftest with an
>   executable loaded), but we acknowledge that we might have to accept
>   some selftest failures in this case, or
> 
>   2. Modify 'maint selftest' so it throws an error if GDB has an
>   executable loaded, write a new test that checks this error occurs,
>   and, write an additional test that reveals the bug I originally saw
>   (via 'maint selftest') but through the alternative route.
> 
>   3. Maybe you have a different suggestion?
> 
> Honestly, I'm prepare to go with any approach, it's just, I found a
> way to crash GDB, I fixed it, and I think we should make sure the same
> failure can't be introduced again in the future.

My opinion would be that we should write selftests in a way that they
can assume they start with a blank slate.  And tests would be
responsible for clearing everything they added, leaving the state blank
as they found it (the selftests runner could verify that).  But Pedro
didn't agree with this (I don't remember the specific reasons), so I
ended up doing some fixes like this one:

   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3ee93972f9dbbb77a7cd4f63c6c7bb66a8b12c71

So I'd say that the current state is that selftests should be prepared
to encounter any state and make sure they leave things as they found it
(e.g. if they change the current inferior, they need to restore that),
and we should follow that.

We could change that guideline, but for that we would need to know the
pros and cons of each.

Simon

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Luis Machado]

On 5/21/21 5:37 AM, Andrew Burgess wrote:
> * Luis Machado <luis.machado@linaro.org> [2021-05-20 13:06:04 -0300]:
> 
>> On 5/20/21 12:41 PM, Andrew Burgess wrote:
>>> * Luis Machado <luis.machado@linaro.org> [2021-05-20 11:29:07 -0300]:
>>>
>>>> On 5/20/21 10:29 AM, Andrew Burgess wrote:
>>>>> I spotted that 'maint selftest' with an executable loaded into GDB,
>>>>> would (when GDB was compiled for all targets) crash GDB.  I fixed this
>>>>> with a commit to bfd:
>>>>
>>>> I don't understand the purpose of being able to run selftests with an
>>>> executable loaded, or, more generally, running selftests when any GDB state
>>>> is capable of interfering with the tests.
>>>>
>>>> Should we force the selftests to cleanup the state before we run them? Or
>>>> not allow running selftests when there is a debugging session already
>>>> stablished?
>>>
>>> I'm not trying to claim such a thing is useful, but I do feel pretty
>>> strongly that running 'maint selftest' (with an executable loaded)
>>> shouldn't cause GDB to segfault.  To ensure it doesn't get broken
>>> again in the future I'd suggest we need add this configuration to the
>>> testsuite.
>>>
>>> I guess one possible solution would have been to add a check within
>>> the 'maint selftest' command that throws an error if an executable is
>>> loaded, but, that would have meant we missed a real bug (I claim) in
>>> bfd.
>>>
>>> You'll need to check out commit 427e4066afd1 for details of the issue
>>> I found, but it could be reproduced (though I admit via a rather weird
>>> set of steps) outside of 'maint selftest'.
>>>
>>> I understand your concerns about the selftests potentially depending
>>> on GDB being in a vanilla state, which might be corrupted by loading
>>> an executable, right now this doesn't seem to be the case (too much, I
>>> guess the ARM failure is an example of this happening).
>>>
>>> If in the future we get more such cases, I'd have no problems with us
>>> increasing the number of acceptable selftest failures.
>>
>> Right. I'm not against the fix, but this shows the selftest design in GDB is
>> kinda broken in this regard. It is not self-contained and seems to rely on
>> external state.
>>
>> Just wanted to point out that we shouldn't need to handle running the
>> selftests with an executable loaded. It makes me wonder what other options
>> we can tweak when GDB is running that will cause the selftests to hit some
>> unpredictable situations.
> 
> You'll have to excuse me for being a little slow today.  I believe I
> understand your concerns, which I think are:
> 
>    Selftest was not designed to be run in any situation other than in a
>    vanilla GDB, with no executable loaded, so it's not surprising that
>    it broke when we did "other" things.
> 
> What I'm struggling with is to translate your concerns into what
> action you think I should take.
> 
> I think I see a couple of possibilities:
> 
>    1. Continue with the test I suggest (running selftest with an
>    executable loaded), but we acknowledge that we might have to accept
>    some selftest failures in this case, or

I'm fine with 1. My point is that if selftests can be influenced by GDB 
global state, we have a bigger design problem. We shouldn't need to be 
fixing these things individually (there may be other hidden issues 
anyway), but rather we need to fix the design that allows selftests to 
be contaminated by GDB's global state.

My take on it is that selftests need to be self-contained.

> 
>    2. Modify 'maint selftest' so it throws an error if GDB has an
>    executable loaded, write a new test that checks this error occurs,
>    and, write an additional test that reveals the bug I originally saw
>    (via 'maint selftest') but through the alternative route.

I don't think you should go out of your way to fix this design flaw, 
unless you feel like doing it. I'd rather not create more work for you. 
We're all busy with other things.

> 
>    3. Maybe you have a different suggestion?
> 
> Honestly, I'm prepare to go with any approach, it's just, I found a
> way to crash GDB, I fixed it, and I think we should make sure the same
> failure can't be introduced again in the future.
> 
> Thanks for all your input,
> 
> Andrew
> 

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Luis Machado]

On 5/21/21 11:25 AM, Simon Marchi wrote:
>> You'll have to excuse me for being a little slow today.  I believe I
>> understand your concerns, which I think are:
>>
>>    Selftest was not designed to be run in any situation other than in a
>>    vanilla GDB, with no executable loaded, so it's not surprising that
>>    it broke when we did "other" things.
>>
>> What I'm struggling with is to translate your concerns into what
>> action you think I should take.
>>
>> I think I see a couple of possibilities:
>>
>>    1. Continue with the test I suggest (running selftest with an
>>    executable loaded), but we acknowledge that we might have to accept
>>    some selftest failures in this case, or
>>
>>    2. Modify 'maint selftest' so it throws an error if GDB has an
>>    executable loaded, write a new test that checks this error occurs,
>>    and, write an additional test that reveals the bug I originally saw
>>    (via 'maint selftest') but through the alternative route.
>>
>>    3. Maybe you have a different suggestion?
>>
>> Honestly, I'm prepare to go with any approach, it's just, I found a
>> way to crash GDB, I fixed it, and I think we should make sure the same
>> failure can't be introduced again in the future.
> 
> My opinion would be that we should write selftests in a way that they
> can assume they start with a blank slate.  And tests would be
> responsible for clearing everything they added, leaving the state blank
> as they found it (the selftests runner could verify that).  But Pedro
> didn't agree with this (I don't remember the specific reasons), so I
> ended up doing some fixes like this one:
> 
>     https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3ee93972f9dbbb77a7cd4f63c6c7bb66a8b12c71
> 
> So I'd say that the current state is that selftests should be prepared
> to encounter any state and make sure they leave things as they found it
> (e.g. if they change the current inferior, they need to restore that),
> and we should follow that.

I find that a bit non-intuitive. That means we'd have some sort of use 
for being able to run GDB up to a certain point and then fire up a 
particular set of selftests that would rely on GDB's global state at 
that point. That seems fairly fragile and confusing.

> 
> We could change that guideline, but for that we would need to know the
> pros and cons of each.
I'm in favor of that. It simplifies writing selftests in my opinion, as 
we don't have to worry about external state.

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> My opinion would be that we should write selftests in a way that they
Simon> can assume they start with a blank slate.

FWIW, back when I added self-tests, my view was that they'd primarily be
useful for testing APIs that didn't rely on any global state.  For
example, my initial use case was testing the Rust lexer.

Of course, that's not really a reasonable position given the reality of
gdb's code, and the convenience of writing self-tests.

Simon> So I'd say that the current state is that selftests should be prepared
Simon> to encounter any state and make sure they leave things as they found it
Simon> (e.g. if they change the current inferior, they need to restore that),
Simon> and we should follow that.

We can try it, and if we run into a problem that can't readily be dealt
with, we can always change our minds then.

In the particular case in question, I think it's fine to add this new
proposed test.

thanks,
Tom

Re: [PATCH] gdb: run 'maint selftest' with an executable loaded

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-05-24 08:06:57 -0600]:

> >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> My opinion would be that we should write selftests in a way that they
> Simon> can assume they start with a blank slate.
> 
> FWIW, back when I added self-tests, my view was that they'd primarily be
> useful for testing APIs that didn't rely on any global state.  For
> example, my initial use case was testing the Rust lexer.
> 
> Of course, that's not really a reasonable position given the reality of
> gdb's code, and the convenience of writing self-tests.
> 
> Simon> So I'd say that the current state is that selftests should be prepared
> Simon> to encounter any state and make sure they leave things as they found it
> Simon> (e.g. if they change the current inferior, they need to restore that),
> Simon> and we should follow that.
> 
> We can try it, and if we run into a problem that can't readily be dealt
> with, we can always change our minds then.
> 
> In the particular case in question, I think it's fine to add this new
> proposed test.

Thank you everyone for your feedback.

I have pushed this change now.  My goal was always to ensure that
'maint test' with an executable doesn't crash GDB, I don't mind if we
later decide that this is a use case that we shouldn't support.
If/when that day comes I would not object if we simply gave an error
in that situation and this test was changed again.

Thanks,
Andrew