RE: [PATCH v3] Support glibc multiple namespace extension

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Kevin,

There's a newer version of this patch
https://sourceware.org/pipermail/gdb-patches/2021-November/183498.html
that already addresses the style concerns you raise.

>> @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info,
>CORE_ADDR lm)
>>    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>>      /* Nothing.  */;
>>
>> -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
>> -  prev_lm = li->lm_addr;
>> +  /* Don't check shared libraries in other namespaces when updating
>> +     shared libraries in a new namespace.  */
>
>Shared libraries in other namespaces aren't being neglected though,
>right?

They're not, but this code ...

  /* Walk to the end of the list.  */
  for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
    /* Nothing.  */;

  /* Don't check shared libraries in other namespaces when updating
     shared libraries in a new namespace.  */
  if (debug_base == info->debug_base)
    {
      lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
      prev_lm = li->lm_addr;
    }
  else
    prev_lm = 0;

... looks a bit odd to me.  We're adding to the same solib_list, yet we use
different prev_lm.  Let me look into this some more.

Regards,
Markus.


>-----Original Message-----
>From: Kevin Buettner <kevinb@redhat.com>
>Sent: Friday, February 11, 2022 12:08 AM
>To: H.J. Lu via Gdb-patches <gdb-patches@sourceware.org>
>Cc: H.J. Lu <hjl.tools@gmail.com>; Metzger, Markus T
><markus.t.metzger@intel.com>
>Subject: Re: [PATCH v3] Support glibc multiple namespace extension
>
>Hi H.J.,
>
>This work looks pretty good to me.  I found a couple of coding style nits
>and have a question/request; see below.
>
>On Sat, 2 Oct 2021 13:43:40 -0700
>"H.J. Lu via Gdb-patches" <gdb-patches@sourceware.org> wrote:
>
>[...]
>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>> index 3de1bb9c7f7..9d851ba8930 100644
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>[...]
>> @@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
>>    if (lm)
>>      svr4_read_so_list (info, lm, 0, &link_ptr, 0);
>>
>> +  /* Get the next namespace from the r_next field.  */
>> +  lm = solib_svr4_r_next (info->debug_base);
>> +  while (lm)
>
>Due to the GDB coding standard, this test needs to be turned into an
>explicit comparison against 0.  See:
>
>https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
>Standards#Comparison_With_nullptr_And_Zero
>
>(FWIW, I don't really like this part of our standard; I personally
>find it more readable to code it the way that you did.  Also, I do
>realize that there are existing implicit tests against 0 in this file
>and elsewhere and that they do not conform to GDB's current coding
>standard.  I'm in no hurry to fix them - but, obviously, I won't
>object if someone does.)
>
>> +    {
>> +      /* Get the link map in this namespace.  */
>> +      CORE_ADDR link_map = solib_svr4_r_map (lm);
>> +      if (link_map)
>
>Likewise, for the check above.
>
>> +	svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
>> +      /* Go to the next namespace.  */
>> +      lm = solib_svr4_r_next (lm);
>> +    }
>> +
>>    cleanup.release ();
>>
>>    if (head == NULL)
>> @@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
>>     failure.  */
>>
>>  static int
>> -solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
>> +solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
>> +			   CORE_ADDR lm)
>>  {
>>    struct so_list *tail;
>>    CORE_ADDR prev_lm;
>> @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info,
>CORE_ADDR lm)
>>    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>>      /* Nothing.  */;
>>
>> -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
>> -  prev_lm = li->lm_addr;
>> +  /* Don't check shared libraries in other namespaces when updating
>> +     shared libraries in a new namespace.  */
>
>Shared libraries in other namespaces aren't being neglected though,
>right?
>
>Assuming that's true, could you add a sentence or two to your comment
>addressing this concern?
>
>Kevin

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928