[3/8] Set general_thread after restart
Commit Message
When I run gdb.server/ext-restart.exp, I get the following GDB internal
error,
run^M
The program being debugged has been started already.^M
Start it from the beginning? (y or n) y^M
Sending packet: $vKill;53c5#3d...Packet received: OK^M
Packet vKill (kill) is supported^M
Sending packet: $vFile:close:6#b6...Packet received: F0^M
Sending packet: $vFile:close:3#b3...Packet received: F0^M
Starting program: /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart ^M
Sending packet: $QDisableRandomization:1#cf...Packet received: OK^M
Sending packet: $R0#82...Sending packet: $qC#b4...Packet received: QCp53c5.53c5^M <-- [1]
Sending packet: $qAttached:53c5#c9...Packet received: E01^M
warning: Remote failure reply: E01^M
....
0x00002aaaaaaac2d0 in ?? () from target:/lib64/ld-linux-x86-64.so.2^M
/home/yao/SourceCode/gnu/gdb/git/gdb/thread.c:88: internal-error: inferior_thread: Assertion `tp' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
Quit this debugging session? (y or n) FAIL: gdb.server/ext-restart.exp: run to main (GDB internal error)
Resyncing due to internal error.
the test is to restart the program, to make sure GDBserver handles
packet 'R' correctly. From the GDBserver output, we can see,
Remote debugging from host 127.0.0.1^M
Process /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart created; pid = 21445^M
GDBserver restarting^M
Process /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart created; pid = 21446^M
Killing process(es): 21446
we first start process 21445(0x53c5), kill it and restart a new process
21446. However, in the gdb output above [1], we can see that the reply
of qC is still the old process id rather than the new one. Looks
general_thread isn't up to date after GDBserver receives R packet.
This patch is to update general_thread after call start_inferior.
gdb/gdbserver:
2015-07-16 Yao Qi <yao.qi@linaro.org>
* server.c (process_serial_event): Set general_thread.
gdb/testsuite:
2015-07-16 Yao Qi <yao.qi@linaro.org>
* gdb.server/ext-restart.exp: New file.
---
gdb/gdbserver/server.c | 5 ++-
gdb/testsuite/gdb.server/ext-restart.exp | 65 ++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.server/ext-restart.exp
Comments
On 07/20/2015 12:35 PM, Yao Qi wrote:
> When I run gdb.server/ext-restart.exp, I get the following GDB internal
> error,
>
> run^M
> The program being debugged has been started already.^M
> Start it from the beginning? (y or n) y^M
> Sending packet: $vKill;53c5#3d...Packet received: OK^M
> Packet vKill (kill) is supported^M
> Sending packet: $vFile:close:6#b6...Packet received: F0^M
> Sending packet: $vFile:close:3#b3...Packet received: F0^M
> Starting program: /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/gdb.server/ext-restart ^M
> Sending packet: $QDisableRandomization:1#cf...Packet received: OK^M
> Sending packet: $R0#82...Sending packet: $qC#b4...Packet received: QCp53c5.53c5^M <-- [1]
> Sending packet: $qAttached:53c5#c9...Packet received: E01^M
> warning: Remote failure reply: E01^M
Curious. Kevin's original patch to handle bogus qC replies would
have hidden this bug.
Plus, today I wrote a patch that exposed a bunch of stale
general_thread issues (but not this one).
Funny how sometimes we all end up staring at the same things
at the same time.
>
> /* Wait till we are at 1st instruction in prog. */
> if (program_argv != NULL)
> - start_inferior (program_argv);
> + {
> + start_inferior (program_argv);
> + general_thread = last_ptid;
> + }
I think this should be:
if (last_status.kind == TARGET_WAITKIND_STOPPED)
{
/* Stopped at the first instruction of the target process. */
general_thread = last_ptid;
}
else
{
/* Something went wrong. */
general_thread = null_ptid;
}
> +# Test running programs using extended-remote.
Comment looks stale. Looks like I missed pointing out the same
in patch #2.
Otherwise looks good to me.
(I think it's likely we have lots of stale-data bugs on the
gdb side after R, as we don't resync much. It previously crossed my mind
that immediately after sending R, gdb should refresh all its
remote state anew, like if it had just disconnected and then reconnected.
That is, do most of what remote_start_remote does, except the
connection-specific details (qSupported, etc.)
Hard to justify the effort though -- I don't think I ever worked with
a stub that relies on R.)
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> I think this should be:
>
> if (last_status.kind == TARGET_WAITKIND_STOPPED)
> {
> /* Stopped at the first instruction of the target process. */
> general_thread = last_ptid;
> }
> else
> {
> /* Something went wrong. */
> general_thread = null_ptid;
> }
>
OK, I'll fix it in the commit ready to push.
>> +# Test running programs using extended-remote.
>
> Comment looks stale. Looks like I missed pointing out the same
> in patch #2.
>
I'll remove it.
> Otherwise looks good to me.
>
> (I think it's likely we have lots of stale-data bugs on the
> gdb side after R, as we don't resync much. It previously crossed my mind
> that immediately after sending R, gdb should refresh all its
> remote state anew, like if it had just disconnected and then reconnected.
> That is, do most of what remote_start_remote does, except the
> connection-specific details (qSupported, etc.)
> Hard to justify the effort though -- I don't think I ever worked with
> a stub that relies on R.)
Even GDB refreshes all its state after sending R packet, we still need
some way to test GDB and GDBserver with R packet used. Otherwise, it
will be bit-rotten in the future.
GDBserver has already had an option --disable-packet, so that we can
extend it to force GDBserver/GDB use R packet. However, I don't think
we use --disable-packet much in our testing, at least I don't. Probably
we can hack native-gdbserver.exp to run tests in a loop and pass
different --disable-packet=FOO to GDBserver in each iteration.
On 07/24/2015 10:33 AM, Yao Qi wrote:
>>> +# Test running programs using extended-remote.
>>
>> Comment looks stale. Looks like I missed pointing out the same
>> in patch #2.
>>
>
> I'll remove it.
I find these descriptions useful. Could you instead write something
like:
"Test restarting programs with the R packet."
?
>
>> Otherwise looks good to me.
>>
>> (I think it's likely we have lots of stale-data bugs on the
>> gdb side after R, as we don't resync much. It previously crossed my mind
>> that immediately after sending R, gdb should refresh all its
>> remote state anew, like if it had just disconnected and then reconnected.
>> That is, do most of what remote_start_remote does, except the
>> connection-specific details (qSupported, etc.)
>> Hard to justify the effort though -- I don't think I ever worked with
>> a stub that relies on R.)
>
> Even GDB refreshes all its state after sending R packet, we still need
> some way to test GDB and GDBserver with R packet used. Otherwise, it
> will be bit-rotten in the future.
Sounds like we're talking past each other.
Not sure what I said that made it sounds like that
idea would obviate the need for the test -- I think your new
test is great.
I meant something like gdb itself, around extended_remote_restart, calling
into a new function factored out from remote_start_remote.
This is because the R packet is documented as having no reply, like
'k', no doubt because it assumes the remote target can really hard reset
after the R packet. But let's forget it; hardly worth it to spend time
on it right now.
>
> GDBserver has already had an option --disable-packet, so that we can
> extend it to force GDBserver/GDB use R packet. However, I don't think
> we use --disable-packet much in our testing, at least I don't. Probably
> we can hack native-gdbserver.exp to run tests in a loop and pass
> different --disable-packet=FOO to GDBserver in each iteration.
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> I find these descriptions useful. Could you instead write something
> like:
>
> "Test restarting programs with the R packet."
>
> ?
OK.
>
>>
>>> Otherwise looks good to me.
>>>
>>> (I think it's likely we have lots of stale-data bugs on the
>>> gdb side after R, as we don't resync much. It previously crossed my mind
>>> that immediately after sending R, gdb should refresh all its
>>> remote state anew, like if it had just disconnected and then reconnected.
>>> That is, do most of what remote_start_remote does, except the
>>> connection-specific details (qSupported, etc.)
>>> Hard to justify the effort though -- I don't think I ever worked with
>>> a stub that relies on R.)
>>
>> Even GDB refreshes all its state after sending R packet, we still need
>> some way to test GDB and GDBserver with R packet used. Otherwise, it
>> will be bit-rotten in the future.
>
> Sounds like we're talking past each other.
> Not sure what I said that made it sounds like that
> idea would obviate the need for the test -- I think your new
> test is great.
No :) I have a habit that think about how to test the change before I
start to change. This leads me there.
>
> I meant something like gdb itself, around extended_remote_restart, calling
> into a new function factored out from remote_start_remote.
> This is because the R packet is documented as having no reply, like
> 'k', no doubt because it assumes the remote target can really hard reset
> after the R packet. But let's forget it; hardly worth it to spend time
> on it right now.
Yes, that is a good idea, and you are right that it is hard to justify
the effort now.
@@ -4121,7 +4121,10 @@ process_serial_event (void)
/* Wait till we are at 1st instruction in prog. */
if (program_argv != NULL)
- start_inferior (program_argv);
+ {
+ start_inferior (program_argv);
+ general_thread = last_ptid;
+ }
else
{
last_status.kind = TARGET_WAITKIND_EXITED;
new file mode 100644
@@ -0,0 +1,65 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2015 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/>.
+
+# Test running programs using extended-remote.
+
+load_lib gdbserver-support.exp
+
+standard_testfile server.c
+
+if { [skip_gdbserver_tests] } {
+ return 0
+}
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile debug] } {
+ return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set target_exec [gdbserver_download_current_prog]
+gdbserver_start_extended
+
+gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+
+gdb_breakpoint main
+gdb_test "run" "Breakpoint.* main .*" "run to main"
+
+# Restart the process.
+with_test_prefix "restart" {
+ # Disable vRun packet and clear remote exec-file, so that GDB will
+ # use R packet to restart the process.
+ gdb_test_no_output "set remote run-packet off"
+ gdb_test_no_output "set remote exec-file"
+
+ set test "run to main"
+ gdb_test_multiple "run" $test {
+ -re {Start it from the beginning\? \(y or n\) $} {
+ send_gdb "y\n"
+ exp_continue
+ }
+ -re "Breakpoint.* main .*\r\n$gdb_prompt $" {
+ pass $test
+ }
+ }
+}
+
+gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
+
+gdb_test_no_output "monitor exit"