public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
To: "simark@simark.ca" <simark@simark.ca>,
	Aditya Kamath1 <Aditya.Kamath1@ibm.com>,
	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch
Date: Wed, 8 Feb 2023 18:44:53 +0000	[thread overview]
Message-ID: <d3b999364f2ee9506879cd7910abe5c676502bbe.camel@de.ibm.com> (raw)
In-Reply-To: <CH2PR15MB35449148A6541A1581B1749ED6DB9@CH2PR15MB3544.namprd15.prod.outlook.com>

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>>This seems unrelated to the rest of the changes at first glance.
>>Why is this necessary?
>
>So, when we need to be in the right context when we read memory. Before
>coming into the target wait, we switch_to_no_thread () due to which our
>inferior_ptid is set to null. Our target_memory needs the correct
>inferior_ptid.  Also, in case we don't have a ptid_t (pid) and the
>application is threaded we need the inferior_ptid to be set correctly
>like shown in the patch.

Understood.

>Previously we used switch_to_thread ().. Now if the application is
>theraded and we only pass ptid_t (user_current_pid) to switch_to_thread ()
>it will crash as main thread looks different or is ptid_t (pid, 0, tid).

This part I don't quite understand yet - how/why does it crash?

>>By comparison, the Linux version of this in proc-service.c also
>>switches the current inferior and address space:
> > scoped_restore_current_inferior restore_inferior;
> > set_current_inferior (ph->thread->inf);
> > scoped_restore_current_program_space restore_current_progspace;
> > set_current_program_space (ph->thread->inf->pspace);
> > scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
> > inferior_ptid = ph->thread->ptid;
>> so we should probably do the same for consistency.

>So, kindly allow me to disagree with you on this. What is happening is in
>inferior.c in do_target_wait1 () we call switch_to_inferior_no_thread ()..
[snip]
>Here we already set the correct current inferior and program space to
>the same thing as that if we set in pdc_read_memory like linux.
>So, it does not make any difference to add the changes like linux does

Well, it does look like if you entered the callback in this particular
context, the inferior may have already been set up correctly.  However,
in theory the callback could also be called in different contexts, and
just as a precaution it would be preferable to have it always work
correctly.  The semantics of the callback is to read memory of a
particular process as identified via the pthdb_user_t argument, and
we should write the routine so that it always does what's needed to
implement that semantics correctly.

>Secondly, things work if we do not do the same for pdc_write_memory.
>I have not seen anything not work. So, I don't think it is good to
>add it there. What say??

Similarly, I agree that everything may currently "work" without
adding the equivalent change to pdc_write_memory, but most likely
this is simply because that callback may just not be used very much.

But as a precaution, and to accommodate potential future changes
e.g. in the libpthdebug.a library, if would be preferable to
implement the semantics correctly.  (Also, it just looks surprising
to see the read and write implementation differ when there is no
apparent reason why that should be the case.)

>>This looks unnecessarily complicated.  Isn't this just
>  > *g++ = tp;
>
>This I have changed. 

The code now looks like:
>+  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
>+  {
>+    *g = tp;
>+    *g++;
>+  }

Which is weird, as *g++ dereferences g for no reason.  This should
simply be:

  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
    *g++ = tp;


>As far as the check gptid.is_pid () is concerned, I will suggest we
>keep it there. If cmp_result is > 0 and we have a main process swap
>to create a thread. Rest is same in the loop. The reason being handling
>pi and gi variables becomes complex otherwise. When this swap happens,
>we need to increment both pi and gi.. Because we have taken care of the
>main threads in both pthread library and GDB. And this for loop is
>executed only once. So, the first event is main process being
>pthreaded. Once the swap happens pi and gi become one and since
>gcount = pcount = 1 we exit the for loop. Thread addition events comes
>after this. 

Hmm, handling the initial switch of a single PID-only thread
to the PID/TID-style ptid_t separately before still seems
a bit clearer to me.  But in the end your proposed code looks
correct now so I'd be fine with it as is, if you prefer.


Except for the few things mentioned above, this now looks ready to
be committed to me.  However, I'm not sure the commit message
fully describes the latest version of the patch, after we've
gone through all those iterations ...  Can you come up with a
message that maybe starts out with the high-level change
(along the lines of "update aix-thread.c to handle threads in
multiple inferiors"), and goes from there into the specific
details (aix_thread_variables structure, handling only a
single inferior per sync_threadlists invocation, solib fixes
for multiple inferiors, ...)?  Thanks!

Bye,
Ulrich

 

  reply	other threads:[~2023-02-08 18:44 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-25  6:47 Aditya Kamath1
2022-10-28  9:49 ` Ulrich Weigand
2022-11-08 12:00   ` Aditya Kamath1
2022-11-08 12:17     ` Ulrich Weigand
2022-11-13 18:15       ` Aditya Kamath1
2022-11-15 18:16         ` Ulrich Weigand
2022-11-21  8:27           ` Aditya Kamath1
2022-11-23 14:15             ` Ulrich Weigand
2022-11-23 16:03               ` Aditya Kamath1
2022-11-23 17:09                 ` Ulrich Weigand
2022-11-23 18:45                   ` Aditya Kamath1
2022-11-29  8:18                     ` Aditya Kamath1
2022-11-30 14:57                       ` Ulrich Weigand
2022-12-02  7:50                         ` Aditya Kamath1
2022-12-05 18:33                           ` Ulrich Weigand
2022-12-08 10:28                             ` Aditya Kamath1
2022-12-08 10:46                               ` Aditya Kamath1
2022-12-08 16:29                               ` Ulrich Weigand
2022-12-15 12:58                                 ` Aditya Kamath1
2022-12-15 15:53                                   ` Ulrich Weigand
2022-12-19  6:30                                     ` Aditya Kamath1
2022-12-22 12:50                                       ` Ulrich Weigand
2022-12-26 13:18                                         ` Aditya Kamath1
2023-01-09 14:04                                           ` Ulrich Weigand
2023-01-10 12:23                                             ` Aditya Kamath1
2023-01-11 13:31                                               ` Ulrich Weigand
2023-01-13 14:06                                                 ` Aditya Kamath1
2023-01-20 14:44                                                   ` Ulrich Weigand
2023-01-27 14:40                                                     ` Aditya Kamath1
2023-01-30 19:54                                                       ` Tom Tromey
2023-02-02  6:24                                                       ` Aditya Kamath1
2023-02-02  6:35                                                         ` Aditya Kamath1
2023-02-02 17:43                                                           ` Ulrich Weigand
2023-02-03 11:10                                                             ` Aditya Kamath1
2023-02-06 19:07                                                               ` Ulrich Weigand
2023-02-07 11:57                                                                 ` Aditya Kamath1
2023-02-08 18:44                                                                   ` Ulrich Weigand [this message]
2023-02-10 16:33                                                                     ` Aditya Kamath1
2023-02-10 16:46                                                                       ` Aditya Kamath1
2023-02-13 19:01                                                                       ` Ulrich Weigand
2023-02-14 14:13                                                                         ` Aditya Kamath1
2023-02-16 19:46                                                                           ` Ulrich Weigand
2023-02-17 11:26                                                                             ` Aditya Kamath1
2023-02-17 12:04                                                                               ` Ulrich Weigand
2023-02-17 13:22                                                                                 ` Aditya Kamath1
2023-02-17 14:18                                                                                   ` Ulrich Weigand
2023-02-17 15:15                                                                                     ` Aditya Kamath1
2023-02-17 19:14                                                                                       ` Ulrich Weigand
2022-11-08 12:00 Aditya Kamath1

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d3b999364f2ee9506879cd7910abe5c676502bbe.camel@de.ibm.com \
    --to=ulrich.weigand@de.ibm.com \
    --cc=Aditya.Kamath1@ibm.com \
    --cc=gdb-patches@sourceware.org \
    --cc=sangamesh.swamy@in.ibm.com \
    --cc=simark@simark.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).