Re: ptid from core section

[Aleksandar Ristovski <aristovski@qnx.com>]

Pedro Alves wrote:
> On Friday 05 June 2009 15:53:00, Aleksandar Ristovski wrote:
> 
>> Here is a new patch. I made sure it works even if 
>> core_gdbarch is NULL. 
> 
> Sorry, we're going a bit in circles.  The way you've now
> implemented it, there are three ways to get a ptid
> from a core reg section, but two of them are mostly
> the same --- the default gdbarch fallbacks, and the 
> case of core_gdbarch being NULL handled in corelow.c itself.

On nto, we are not using lwp field at all. We use thread id. 
  In nto, I override core_open to add thread private data 
once core_open has finished it's work. To identify threads, 
I need to build ptid the way core does. I can do that, but 
then I will have two functions for adding thread private 
data (one for core ops, one for live target). Also, I would 
have to either figure out which field is being used in 
*_ptid_to_str or again provide two functions.

In summary: the motivation for this patch is to avoid having 
to patch corelow.c. I am working on a larger nto patch that 
I would like to have in before next official gdb release.

> 
> On Friday 05 June 2009 14:44:37, Pedro Alves wrote:
>> change adds some unconditional accesses.  The path of
>> least resistence to fix this, is to move the callback defaults
>> to corelow.c, make the new callbacks optional, and check
>> for 'core_gdbarch && gdbarch_foo_p (core_gdbarch)' predicates
>> before calling the optional callbacks.
> 
> This meant:
> 
> On Friday 05 June 2009 15:53:00, Aleksandar Ristovski wrote:
>> +static ptid_t
>> +default_ptid_from_core_section_name (struct gdbarch *gdbarch, const bfd *abfd,
>> +                                    const char *name)
>> +{
>> +  int thread_id;
>> +  ptid_t ptid;
>> +  const char *pos;
>> +
>> +  pos = strchr (name, '/'); 
>> +  if (pos == NULL)
>> +    pos = name + strlen (name);
>> +  else
>> +    pos++;
>> +  thread_id = atoi (pos);
>> +  ptid = ptid_build (ptid_get_pid (inferior_ptid), thread_id, 0);
>> +  return ptid;
>> +}
>> +
>> +static char *
>> +default_core_section_name_from_ptid (struct gdbarch *gdbarch, 
>> +                                    const bfd *abfd,
>> +                                    const char *name,
>> +                                    ptid_t ptid)
>> +{
>> +  if (ptid_get_lwp (ptid))
>> +    return xstrprintf ("%s/%ld", name, ptid_get_lwp (ptid));
>> +  else
>> +    return xstrdup (name);
>> +}
> 
> Moving these functions to corelow.c, and making the gdbarch callbacks
> optional.  Wouldn't that look cleaner?  Something like the patch below.

Looking at core handling I think the whole thing is not 
clean. I see your point, but I see no particular advantage 
of your approach - we moved the default code to corelow but 
now introduced check for gdbarch_..._p.

But don't get me wrong - I am not against your approach - as 
long as I don't have to patch corelow in order to get gdb 
working for neutrino.



> 
> But, at this point, I'm now confused, and I have to re-ask:
> What is it that gets confused on nto when core files store
> the thread id in the lwp field of ptids instead of on the tid
> field?  Your original patch only took care to adjust to
> read tids from the tid field in default_core_section_from_ptid,
> but didn't do anything to make ptids that stored the tid
> in the tid field in default_ptid_from_core_section?  While
> your latest patch didn't even do that in
> default_ptid_from_core_section_name?  I can't see how you
> avoid adding gdbarch callbacks for nto.

I don't, I provide my own callbacks similar to sol2-tdep.c, 
only they build ptid something like this:

ptid_build (ptid_get_pid (ptid), 0, atoi (core_section_name 
+ 5));

I reworked the latest patch to have exactly the same 
behaviour as without the patch, except for one bit where I 
check if both lwp and tid are 0 to set inferior_ptid.


> 
>> I also fixed my previous patch for  
>> target signal by checking if there is a core_gdbarch.
> 
> Could you go ahead, and check in just that bit
> split out from the rest please?  Thanks.
> 

Sure.


-- 
Aleksandar Ristovski
QNX Software Systems