[PATCH][gdb] Mention CU offset for <artifical> if verbose

[PATCH][gdb] Mention CU offset for <artifical> if verbose

[Tom de Vries]

Hi,

Say we're debugging a test-case with CUs with name "<artificial>", meaning
not originating from a single file compilation, and use the verbose setting:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>... \
  and /tmp/trunk/gcc/attribs.c... \
  ...
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

From the "/tmp/trunk/gcc/attribs.c" message, it's clear which CU is loaded.  But
that's not the case for the "<artificial>" message.

The message uses the filename field of struct partial_symtab, which is
documented like this:
...
  /* Name of the source file which this partial_symtab defines,
     or if the psymtab is anonymous then a descriptive name for
     debugging purposes, or "".  It must not be NULL.  */
...

So, fix this by setting the filename field to a more descriptive name than
"<artificial>", by appending the CU offset.

This way, we print instead:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>@0x41146d9 \
  and /tmp/trunk/gcc/attribs.c... \
  ... \
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

Build and reg-tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb] Mention CU offset for <artifical> if verbose

gdb/ChangeLog:

2020-02-07  Tom de Vries  <tdevries@suse.de>

	* dwarf2read.c (create_partial_symtab): Append CU offset to filename
	if it matches "<artificial>".

---
 gdb/dwarf2read.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index dafe01d94a..28ade424fd 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8020,6 +8020,20 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
   struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
   dwarf2_psymtab *pst;
 
+  const char *artificial = "<artificial>";
+  if (strcmp (name, artificial) == 0)
+    {
+      sect_offset cu_offset = per_cu->sect_off;
+      const char *cu_offset_str = sect_offset_str (cu_offset);
+      const char *sep = "@";
+      char *new_name = (char *) xmalloc (strlen (artificial) + strlen (sep)
+					 + strlen (cu_offset_str) + 1);
+      strcpy (new_name, artificial);
+      strcat (new_name, sep);
+      strcat (new_name, cu_offset_str);
+      name = new_name;
+    }
+
   pst = new dwarf2_psymtab (name, objfile, 0);
 
   pst->psymtabs_addrmap_supported = true;

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Christian Biesinger via gdb-patches]

On Fri, Feb 7, 2020 at 6:34 AM Tom de Vries <tdevries@suse.de> wrote:


> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index dafe01d94a..28ade424fd 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -8020,6 +8020,20 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
>    struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
>    dwarf2_psymtab *pst;
>
> +  const char *artificial = "<artificial>";

This may not matter but I'd use static const char artificial[] = "..."

> +  if (strcmp (name, artificial) == 0)
> +    {
> +      sect_offset cu_offset = per_cu->sect_off;
> +      const char *cu_offset_str = sect_offset_str (cu_offset);
> +      const char *sep = "@";
> +      char *new_name = (char *) xmalloc (strlen (artificial) + strlen (sep)
> +                                        + strlen (cu_offset_str) + 1);
> +      strcpy (new_name, artificial);
> +      strcat (new_name, sep);
> +      strcat (new_name, cu_offset_str);

Use concat() instead of malloc/strcpy/strcat?

> +      name = new_name;
> +    }
> +
>    pst = new dwarf2_psymtab (name, objfile, 0);
>
>    pst->psymtabs_addrmap_supported = true;

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On 07-02-2020 15:45, Christian Biesinger wrote:
> On Fri, Feb 7, 2020 at 6:34 AM Tom de Vries <tdevries@suse.de> wrote:
> 
> 
>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index dafe01d94a..28ade424fd 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -8020,6 +8020,20 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
>>    struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
>>    dwarf2_psymtab *pst;
>>
>> +  const char *artificial = "<artificial>";
> 
> This may not matter but I'd use static const char artificial[] = "..."
> 

Done.

>> +  if (strcmp (name, artificial) == 0)
>> +    {
>> +      sect_offset cu_offset = per_cu->sect_off;
>> +      const char *cu_offset_str = sect_offset_str (cu_offset);
>> +      const char *sep = "@";
>> +      char *new_name = (char *) xmalloc (strlen (artificial) + strlen (sep)
>> +                                        + strlen (cu_offset_str) + 1);
>> +      strcpy (new_name, artificial);
>> +      strcat (new_name, sep);
>> +      strcat (new_name, cu_offset_str);
> 
> Use concat() instead of malloc/strcpy/strcat?
> 

Done.

Updated patch re-tested and attached.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-Mention-CU-offset-for-artifical-if-verbose.patch --]
[-- Type: text/x-patch, Size: 2063 bytes --]

[gdb] Mention CU offset for <artifical> if verbose

Say we're debugging a test-case with CUs with name "<artificial>", meaning
not originating from a single file compilation, and use the verbose setting:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>... \
  and /tmp/trunk/gcc/attribs.c... \
  ...
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

From the "/tmp/trunk/gcc/attribs.c" message, it's clear which CU is loaded.  But
that's not the case for the "<artificial>" message.

The message uses the filename field of struct partial_symtab, which is
documented like this:
...
  /* Name of the source file which this partial_symtab defines,
     or if the psymtab is anonymous then a descriptive name for
     debugging purposes, or "".  It must not be NULL.  */
...

So, fix this by setting the filename field to a more descriptive name than
"<artificial>", by appending the CU offset.

This way, we print instead:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>@0x41146d9 \
  and /tmp/trunk/gcc/attribs.c... \
  ... \
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

Build and reg-tested on x86_64-linux.

gdb/ChangeLog:

2020-02-07  Tom de Vries  <tdevries@suse.de>

	* dwarf2read.c (create_partial_symtab): Append CU offset to filename
	if it matches "<artificial>".

---
 gdb/dwarf2read.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index dafe01d94a..5acae3eab8 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8020,6 +8020,10 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
   struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
   dwarf2_psymtab *pst;
 
+  static const char artificial[] = "<artificial>";
+  if (strcmp (name, artificial) == 0)
+    name = concat (artificial, "@", sect_offset_str (per_cu->sect_off), NULL);
+
   pst = new dwarf2_psymtab (name, objfile, 0);
 
   pst->psymtabs_addrmap_supported = true;

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Simon Marchi]

On 2020-02-08 4:16 a.m., Tom de Vries wrote:
> On 07-02-2020 15:45, Christian Biesinger wrote:
>> On Fri, Feb 7, 2020 at 6:34 AM Tom de Vries <tdevries@suse.de> wrote:
>>
>>
>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>> index dafe01d94a..28ade424fd 100644
>>> --- a/gdb/dwarf2read.c
>>> +++ b/gdb/dwarf2read.c
>>> @@ -8020,6 +8020,20 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
>>>    struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
>>>    dwarf2_psymtab *pst;
>>>
>>> +  const char *artificial = "<artificial>";
>>
>> This may not matter but I'd use static const char artificial[] = "..."
>>
> 
> Done.
> 
>>> +  if (strcmp (name, artificial) == 0)
>>> +    {
>>> +      sect_offset cu_offset = per_cu->sect_off;
>>> +      const char *cu_offset_str = sect_offset_str (cu_offset);
>>> +      const char *sep = "@";
>>> +      char *new_name = (char *) xmalloc (strlen (artificial) + strlen (sep)
>>> +                                        + strlen (cu_offset_str) + 1);
>>> +      strcpy (new_name, artificial);
>>> +      strcat (new_name, sep);
>>> +      strcat (new_name, cu_offset_str);
>>
>> Use concat() instead of malloc/strcpy/strcat?
>>
> 
> Done.
> 
> Updated patch re-tested and attached.

Should the string allocated with concat be freed?

Simon

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On 08-02-2020 16:48, Simon Marchi wrote:
> On 2020-02-08 4:16 a.m., Tom de Vries wrote:
>> On 07-02-2020 15:45, Christian Biesinger wrote:
>>> On Fri, Feb 7, 2020 at 6:34 AM Tom de Vries <tdevries@suse.de> wrote:
>>>
>>>
>>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>>> index dafe01d94a..28ade424fd 100644
>>>> --- a/gdb/dwarf2read.c
>>>> +++ b/gdb/dwarf2read.c
>>>> @@ -8020,6 +8020,20 @@ create_partial_symtab (struct dwarf2_per_cu_data *per_cu, const char *name)
>>>>    struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
>>>>    dwarf2_psymtab *pst;
>>>>
>>>> +  const char *artificial = "<artificial>";
>>>
>>> This may not matter but I'd use static const char artificial[] = "..."
>>>
>>
>> Done.
>>
>>>> +  if (strcmp (name, artificial) == 0)
>>>> +    {
>>>> +      sect_offset cu_offset = per_cu->sect_off;
>>>> +      const char *cu_offset_str = sect_offset_str (cu_offset);
>>>> +      const char *sep = "@";
>>>> +      char *new_name = (char *) xmalloc (strlen (artificial) + strlen (sep)
>>>> +                                        + strlen (cu_offset_str) + 1);
>>>> +      strcpy (new_name, artificial);
>>>> +      strcat (new_name, sep);
>>>> +      strcat (new_name, cu_offset_str);
>>>
>>> Use concat() instead of malloc/strcpy/strcat?
>>>
>>
>> Done.
>>
>> Updated patch re-tested and attached.
> 
> Should the string allocated with concat be freed?

Indeed.

I've done that now by using gdb::unique_xmalloc_ptr<char>. [ FWIW,
there's some code in create_type_unit_group that does something similar,
but there we use std::string and string_printf. ]

Also, I realized that the code was too deep in the call stack, and I've
brought it one level up, to process_psymtab_comp_unit_reader, to make
sure it doesn't trigger for f.i. create_type_unit_group.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-Mention-CU-offset-for-artifical-if-verbose.patch --]
[-- Type: text/x-patch, Size: 2317 bytes --]

[gdb] Mention CU offset for <artifical> if verbose

Say we're debugging a test-case with CUs with name "<artificial>", meaning
not originating from a single file compilation, and use the verbose setting:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>... \
  and /tmp/trunk/gcc/attribs.c... \
  ...
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

From the "/tmp/trunk/gcc/attribs.c" message, it's clear which CU is loaded.  But
that's not the case for the "<artificial>" message.

The message uses the filename field of struct partial_symtab, which is
documented like this:
...
  /* Name of the source file which this partial_symtab defines,
     or if the psymtab is anonymous then a descriptive name for
     debugging purposes, or "".  It must not be NULL.  */
...

So, fix this by setting the filename field to a more descriptive name than
"<artificial>", by appending the CU offset.

This way, we print instead:
...
$ gdb -iex "set verbose on" -batch cc1
Reading symbols from cc1...
Reading in symbols for <artificial>@0x41146d9 \
  and /tmp/trunk/gcc/attribs.c... \
  ... \
  and /tmp/trunk/gcc/tree-ssa-reassoc.c... \
  done.
...

Build and reg-tested on x86_64-linux.

gdb/ChangeLog:

2020-02-07  Tom de Vries  <tdevries@suse.de>

	* dwarf2read.c (process_psymtab_comp_unit_reader): Append CU offset to
	filename if it matches "<artificial>".

---
 gdb/dwarf2read.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index dafe01d94a..bd53dcc711 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8059,9 +8059,17 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
   prepare_one_comp_unit (cu, comp_unit_die, pretend_language);
 
   /* Allocate a new partial symbol table structure.  */
+  gdb::unique_xmalloc_ptr<char> debug_filename;
+  static const char artificial[] = "<artificial>";
   filename = dwarf2_string_attr (comp_unit_die, DW_AT_name, cu);
   if (filename == NULL)
     filename = "";
+  else if (strcmp (filename, artificial) == 0)
+    {
+      debug_filename.reset (concat (artificial, "@",
+				    sect_offset_str (per_cu->sect_off), NULL));
+      filename = debug_filename.get ();
+    }
 
   pst = create_partial_symtab (per_cu, filename);
 

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Simon Marchi]

On 2020-02-09 6:48 a.m., Tom de Vries wrote:
> I've done that now by using gdb::unique_xmalloc_ptr<char>. [ FWIW,
> there's some code in create_type_unit_group that does something similar,
> but there we use std::string and string_printf. ]

You could use std::string and string_printf here as well, but what you did
is fine.  The patch LGTM.

Thanks!

> 
> Also, I realized that the code was too deep in the call stack, and I've
> brought it one level up, to process_psymtab_comp_unit_reader, to make
> sure it doesn't trigger for f.i. create_type_unit_group.
> 
> Thanks,
> - Tom

Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +      debug_filename.reset (concat (artificial, "@",
Tom> +				    sect_offset_str (per_cu->sect_off), NULL));

When passing NULL to concat, you either need nullptr or (char *) NULL.

Tom

[PATCH][gdb] Fix -Wstrict-null-sentinel warnings

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

[ was: Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose ]

On 09-02-2020 14:35, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +      debug_filename.reset (concat (artificial, "@",
> Tom> +				    sect_offset_str (per_cu->sect_off), NULL));
> 
> When passing NULL to concat, you either need nullptr or (char *) NULL.

Fixed in attached patch.

OK for trunk?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-Fix-Wstrict-null-sentinel-warnings.patch --]
[-- Type: text/x-patch, Size: 1593 bytes --]

[gdb] Fix -Wstrict-null-sentinel warnings

When passed in CXXFLAGS, -Wstrict-null-sentinel triggers twice in a
gdb/gdbserver build.

Fix the two occurrences.

Build and reg-tested on x86_64-linux.

gdb/ChangeLog:

2020-02-10  Tom de Vries  <tdevries@suse.de>

	* dwarf2/read.c (process_psymtab_comp_unit_reader): Cast concat NULL
	sentinel to char *.

gdbsupport/ChangeLog:

2020-02-10  Tom de Vries  <tdevries@suse.de>

	* environ.c (gdb_environ::set): Cast concat NULL sentinel to char *.

---
 gdb/dwarf2/read.c    | 3 ++-
 gdbsupport/environ.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 9e66e613cd..a7646c87f3 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7260,7 +7260,8 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
   else if (strcmp (filename, artificial) == 0)
     {
       debug_filename.reset (concat (artificial, "@",
-				    sect_offset_str (per_cu->sect_off), NULL));
+				    sect_offset_str (per_cu->sect_off),
+				    (char *)NULL));
       filename = debug_filename.get ();
     }
 
diff --git a/gdbsupport/environ.c b/gdbsupport/environ.c
index 55d0a74c37..a618cf0f6f 100644
--- a/gdbsupport/environ.c
+++ b/gdbsupport/environ.c
@@ -105,7 +105,7 @@ gdb_environ::get (const char *var) const
 void
 gdb_environ::set (const char *var, const char *value)
 {
-  char *fullvar = concat (var, "=", value, NULL);
+  char *fullvar = concat (var, "=", value, (char *)NULL);
 
   /* We have to unset the variable in the vector if it exists.  */
   unset (var, false);

Re: [PATCH][gdb] Fix -Wstrict-null-sentinel warnings

[Simon Marchi]

On 2020-02-10 5:18 a.m., Tom de Vries wrote:
> [ was: Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose ]
> 
> On 09-02-2020 14:35, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> +      debug_filename.reset (concat (artificial, "@",
>> Tom> +				    sect_offset_str (per_cu->sect_off), NULL));
>>
>> When passing NULL to concat, you either need nullptr or (char *) NULL.
> 
> Fixed in attached patch.
> 
> OK for trunk?

I think we usually put a space after the cast:

  (char *) NULL

Also, if we want to enforce this warning, it should be added to gdb/warning.m4.  Makefiles
in gdb/ and gdbserver/ should be re-generated.  gdbsupport/ doesn't use AM_GDB_WARNINGS at
the moment.

Simon

Re: [PATCH][gdb] Fix -Wstrict-null-sentinel warnings

[Tom de Vries]

On 10-02-2020 15:08, Simon Marchi wrote:
> On 2020-02-10 5:18 a.m., Tom de Vries wrote:
>> [ was: Re: [PATCH][gdb] Mention CU offset for <artifical> if verbose ]
>>
>> On 09-02-2020 14:35, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> +      debug_filename.reset (concat (artificial, "@",
>>> Tom> +				    sect_offset_str (per_cu->sect_off), NULL));
>>>
>>> When passing NULL to concat, you either need nullptr or (char *) NULL.
>>
>> Fixed in attached patch.
>>
>> OK for trunk?
> 
> I think we usually put a space after the cast:
> 
>   (char *) NULL
> 

Ack, committed with that fixed.

Thanks,
- Tom

> Also, if we want to enforce this warning, it should be added to gdb/warning.m4.  Makefiles
> in gdb/ and gdbserver/ should be re-generated.  gdbsupport/ doesn't use AM_GDB_WARNINGS at
> the moment.

Re: [PATCH][gdb] Fix -Wstrict-null-sentinel warnings

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Also, if we want to enforce this warning, it should be added to
Simon> gdb/warning.m4.  Makefiles in gdb/ and gdbserver/ should be
Simon> re-generated.

I have a patch for this that I will send shortly.

I didn't know about this warning, or I would have added it earlier :-)

Tom