diff mbox

[3/4] Add thread after updating gdbarch when exec'ing

Message ID 1503828934-26404-4-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Aug. 27, 2017, 10:15 a.m. UTC 
  As mentioned in the previous patch, we should avoid doing register reads
after a process does an exec and before we've updated that inferior's
gdbarch.  Otherwise, we may interpret the registers using the wrong
architecture.  When a process does an exec with "follow-exec-mode new",
a new inferior is added by follow_exec.  The gdbarch of that new
inferior is at first set to some default value, probably specific to the
gdb build (I get "i386" here), which may not be the right one.  It is
updated later by the call to target_find_description.  Before that
point, if we try to read the inferior's registers, we may not interpret
them correctly.  This has been exposed by a failure in
gdb.base/foll-exec-mode.exp after the previous patch, with:

  Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)

The call to "add_thread" done just after adding the inferior is
problematic, because it ends up reading the registers (because the ptid
is re-used, we end up doing a switch_to_thread to it, which tries to
update stop_pc).  The registers returned by gdbserver are the x86-64
ones, while we try to interpret them using the "i386" gdbarch.

Postponing the call to add_thread to until the target
description/gdbarch has been updated seems to fix the issue.

As to why this issue was uncovered by the previous patch: what I think
happened before that patch is that since we were updating stop_pc before
switching to the new inferior, we were filling the regcache associated
to the ptid (this worked fine as long as the architectures of the
previous and new process images were the same).  The call to
switch_to_thread then worked, because the register read hit the
regcache.  Now, it triggers a register read, while the gdbarch is not
set correctly, leading to the "reply is too long" error.  If this is
right, it sounds wrong that we delete and re-add a thread with the same
ptid, and are able to access the registers from the deleted thread.
When we delete a thread, should we clear the regcache associated to that
ptid, so that the new thread starts with a fresh/empty regcache?

gdb/ChangeLog:

	* infrun.c (follow_exec): Call add_thread after
	target_find_description.
---
 gdb/infrun.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Yao Qi Sept. 5, 2017, 10:37 a.m. UTC | #1 
On 17-08-27 12:15:33, Simon Marchi wrote:
> As mentioned in the previous patch, we should avoid doing register reads
> after a process does an exec and before we've updated that inferior's
> gdbarch.  Otherwise, we may interpret the registers using the wrong
> architecture.  When a process does an exec with "follow-exec-mode new",
> a new inferior is added by follow_exec.  The gdbarch of that new
> inferior is at first set to some default value, probably specific to the
> gdb build (I get "i386" here), which may not be the right one.  It is
> updated later by the call to target_find_description.  Before that
> point, if we try to read the inferior's registers, we may not interpret
> them correctly.  This has been exposed by a failure in
> gdb.base/foll-exec-mode.exp after the previous patch, with:
> 
>   Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)
> 
> The call to "add_thread" done just after adding the inferior is
> problematic, because it ends up reading the registers (because the ptid
> is re-used, we end up doing a switch_to_thread to it, which tries to
> update stop_pc).  The registers returned by gdbserver are the x86-64
> ones, while we try to interpret them using the "i386" gdbarch.

The analysis is great!

> 
> Postponing the call to add_thread to until the target
> description/gdbarch has been updated seems to fix the issue.

This imposes an odd restriction on using add_thread, that is, we must
keep in mind that we can't use add_thread until the inferior's gdbarch
or target description is updated.  The question in my mind is that why
do we need to update stop_pc in add_thread? or can we remove stop_pc?
Simon Marchi Sept. 5, 2017, 3:30 p.m. UTC | #2 
On 2017-09-05 12:37 PM, Yao Qi wrote:
> On 17-08-27 12:15:33, Simon Marchi wrote:
>> As mentioned in the previous patch, we should avoid doing register reads
>> after a process does an exec and before we've updated that inferior's
>> gdbarch.  Otherwise, we may interpret the registers using the wrong
>> architecture.  When a process does an exec with "follow-exec-mode new",
>> a new inferior is added by follow_exec.  The gdbarch of that new
>> inferior is at first set to some default value, probably specific to the
>> gdb build (I get "i386" here), which may not be the right one.  It is
>> updated later by the call to target_find_description.  Before that
>> point, if we try to read the inferior's registers, we may not interpret
>> them correctly.  This has been exposed by a failure in
>> gdb.base/foll-exec-mode.exp after the previous patch, with:
>>
>>   Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)
>>
>> The call to "add_thread" done just after adding the inferior is
>> problematic, because it ends up reading the registers (because the ptid
>> is re-used, we end up doing a switch_to_thread to it, which tries to
>> update stop_pc).  The registers returned by gdbserver are the x86-64
>> ones, while we try to interpret them using the "i386" gdbarch.
> 
> The analysis is great!
> 
>>
>> Postponing the call to add_thread to until the target
>> description/gdbarch has been updated seems to fix the issue.
> 
> This imposes an odd restriction on using add_thread, that is, we must
> keep in mind that we can't use add_thread until the inferior's gdbarch
> or target description is updated.  The question in my mind is that why
> do we need to update stop_pc in add_thread? or can we remove stop_pc?

Instinctively, I'd say that we should probably get rid of stop_pc, and instead
get the same info through the current thread instead.

I have a question for you, since you know much more about tdesc than me.  Installing
a default tdesc (e.g. i386) that is replaced just after with the right tdesc
(e.g. i386:x86-64) creates the risk, like in this case, of using the wrong tdesc.
Do you think it would be feasible (and a good idea) to instead have no tdesc
installed until we figure out which one to use?  We could then catch other operations
that are done while the wrong tdesc is present.

For now I'll push this patchset, so that it is in 8.1, but I feel it's just
covering the real problem of having the wrong tdesc installed.

Thanks for the review,

Simon
Simon Marchi Sept. 5, 2017, 3:44 p.m. UTC | #3 
On 2017-09-05 05:30 PM, Simon Marchi wrote:
> For now I'll push this patchset, so that it is in 8.1, but I feel it's just
> covering the real problem of having the wrong tdesc installed.

Err, this should have said 8.0.1 and not 8.1.
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index de0605f..25beaf4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1211,7 +1211,6 @@  follow_exec (ptid_t ptid, char *exec_file_target)
 
       set_current_inferior (inf);
       set_current_program_space (inf->pspace);
-      add_thread (ptid);
     }
   else
     {
@@ -1243,6 +1242,11 @@  follow_exec (ptid_t ptid, char *exec_file_target)
      registers.  */
   target_find_description ();
 
+  /* The add_thread call ends up reading registers, so do it after updating the
+     target description.  */
+  if (follow_exec_mode_string == follow_exec_mode_new)
+    add_thread (ptid);
+
   solib_create_inferior_hook (0);
 
   jit_inferior_created_hook ();