[7/7] Add handling for 64-bit module addresses in Cygwin core dumps

Message ID 20200701213225.14144-8-jon.turney@dronecode.org.uk
State New
Headers show
Series
  • Add gdb support for Cygwin x86_64 core dumps
Related show

Commit Message

Jon Turney July 1, 2020, 9:32 p.m.
bfd/ChangeLog:

2020-07-01  Jon Turney  <jon.turney@dronecode.org.uk>

	* elf.c (elfcore_grok_win32pstatus): Handle NOTE_INFO_MODULE64.

gdb/ChangeLog:

2020-07-01  Jon Turney  <jon.turney@dronecode.org.uk>

	* windows-tdep.c (core_process_module_section): Handle
	NOTE_INFO_MODULE64.
---
 bfd/ChangeLog      |  4 ++++
 bfd/elf.c          | 15 ++++++++++++---
 gdb/ChangeLog      |  5 +++++
 gdb/windows-tdep.c | 24 +++++++++++++++++-------
 4 files changed, 38 insertions(+), 10 deletions(-)

-- 
2.27.0

Comments

Shahab Vahedi via Gdb-patches July 6, 2020, 8:13 p.m. | #1
On Wed, Jul 1, 2020 at 4:34 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:
> --- a/bfd/elf.c

> +++ b/bfd/elf.c

> @@ -10185,10 +10185,19 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)

>        break;

>

>      case 3 /* NOTE_INFO_MODULE */:

> -      /* Make a ".module/xxxxxxxx" section.  */

> +    case 4 /* NOTE_INFO_MODULE64 */:


It really seems like these should be actual constants, which would
also make it easier to understand...

> +      /* Make a ".module/<base_address>" section.  */

>        /* module_info.base_address */

> -      base_addr = bfd_get_32 (abfd, note->descdata + 4);

> -      sprintf (buf, ".module/%08lx", (unsigned long) base_addr);

> +      if (type == 3)


...this if.

> --- a/gdb/windows-tdep.c

> +++ b/gdb/windows-tdep.c

> +  if (data_type == 3) /* NOTE_INFO_MODULE */


Same here.
Jon Turney July 8, 2020, 3:50 p.m. | #2
On 06/07/2020 21:13, Christian Biesinger via Gdb-patches wrote:
> On Wed, Jul 1, 2020 at 4:34 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:

>> --- a/bfd/elf.c

>> +++ b/bfd/elf.c

>> @@ -10185,10 +10185,19 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)

>>         break;

>>

>>       case 3 /* NOTE_INFO_MODULE */:

>> -      /* Make a ".module/xxxxxxxx" section.  */

>> +    case 4 /* NOTE_INFO_MODULE64 */:

> 

> It really seems like these should be actual constants, which would

> also make it easier to understand...


Absolutely.

Before [1], this structure was defined by including Cygwin's 
<sys/procfs.h> (so this code didn't work in cross-environments).

I can't find a model of adding note structure definitions (the other 
grok_note functions seem to rely on OS definitions), so I'm not sure 
what kind of change you are asking me to make in this patch.

[1] 
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=4a6636fbb5c051fbe15d18a005a2a505ef652571

> 

>> +      /* Make a ".module/<base_address>" section.  */

>>         /* module_info.base_address */

>> -      base_addr = bfd_get_32 (abfd, note->descdata + 4);

>> -      sprintf (buf, ".module/%08lx", (unsigned long) base_addr);

>> +      if (type == 3)

> 

> ...this if.

> 

>> --- a/gdb/windows-tdep.c

>> +++ b/gdb/windows-tdep.c

>> +  if (data_type == 3) /* NOTE_INFO_MODULE */

> 

> Same here.
Shahab Vahedi via Gdb-patches July 8, 2020, 4:11 p.m. | #3
On Wed, Jul 8, 2020 at 10:50 AM Jon Turney <jon.turney@dronecode.org.uk> wrote:
>

> On 06/07/2020 21:13, Christian Biesinger via Gdb-patches wrote:

> > On Wed, Jul 1, 2020 at 4:34 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:

> >> --- a/bfd/elf.c

> >> +++ b/bfd/elf.c

> >> @@ -10185,10 +10185,19 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)

> >>         break;

> >>

> >>       case 3 /* NOTE_INFO_MODULE */:

> >> -      /* Make a ".module/xxxxxxxx" section.  */

> >> +    case 4 /* NOTE_INFO_MODULE64 */:

> >

> > It really seems like these should be actual constants, which would

> > also make it easier to understand...

>

> Absolutely.

>

> Before [1], this structure was defined by including Cygwin's

> <sys/procfs.h> (so this code didn't work in cross-environments).

>

> I can't find a model of adding note structure definitions (the other

> grok_note functions seem to rely on OS definitions), so I'm not sure

> what kind of change you are asking me to make in this patch.


I was just suggesting adding a `static constexpr int
NOTE_INFO_MODULE64 = 4` in some appropriate place, maybe at the top of
this file.

Er, I guess this is bfd, so make it a #define.

>

> [1]

> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=4a6636fbb5c051fbe15d18a005a2a505ef652571

>

> >

> >> +      /* Make a ".module/<base_address>" section.  */

> >>         /* module_info.base_address */

> >> -      base_addr = bfd_get_32 (abfd, note->descdata + 4);

> >> -      sprintf (buf, ".module/%08lx", (unsigned long) base_addr);

> >> +      if (type == 3)

> >

> > ...this if.

> >

> >> --- a/gdb/windows-tdep.c

> >> +++ b/gdb/windows-tdep.c

> >> +  if (data_type == 3) /* NOTE_INFO_MODULE */

> >

> > Same here.
Jon Turney July 12, 2020, 12:58 p.m. | #4
On 08/07/2020 17:11, Christian Biesinger via Gdb-patches wrote:
> On Wed, Jul 8, 2020 at 10:50 AM Jon Turney <jon.turney@dronecode.org.uk> wrote:

>> On 06/07/2020 21:13, Christian Biesinger via Gdb-patches wrote:

>>> On Wed, Jul 1, 2020 at 4:34 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:

>>>> --- a/bfd/elf.c

>>>> +++ b/bfd/elf.c

>>>> @@ -10185,10 +10185,19 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)

>>>>          break;

>>>>

>>>>        case 3 /* NOTE_INFO_MODULE */:

>>>> -      /* Make a ".module/xxxxxxxx" section.  */

>>>> +    case 4 /* NOTE_INFO_MODULE64 */:

>>>

>>> It really seems like these should be actual constants, which would

>>> also make it easier to understand...

>>

>> Absolutely.

>>

>> Before [1], this structure was defined by including Cygwin's

>> <sys/procfs.h> (so this code didn't work in cross-environments).

>>

>> I can't find a model of adding note structure definitions (the other

>> grok_note functions seem to rely on OS definitions), so I'm not sure

>> what kind of change you are asking me to make in this patch.

> 

> I was just suggesting adding a `static constexpr int

> NOTE_INFO_MODULE64 = 4` in some appropriate place, maybe at the top of

> this file.

> 

> Er, I guess this is bfd, so make it a #define.


I posted a revised patchset with that change (to the binutils list, 
where it belongs).

Patch

diff --git a/bfd/elf.c b/bfd/elf.c
index a61e2b7dd1d..00858e16fd3 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -10185,10 +10185,19 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
       break;
 
     case 3 /* NOTE_INFO_MODULE */:
-      /* Make a ".module/xxxxxxxx" section.  */
+    case 4 /* NOTE_INFO_MODULE64 */:
+      /* Make a ".module/<base_address>" section.  */
       /* module_info.base_address */
-      base_addr = bfd_get_32 (abfd, note->descdata + 4);
-      sprintf (buf, ".module/%08lx", (unsigned long) base_addr);
+      if (type == 3)
+        {
+          base_addr = bfd_get_32 (abfd, note->descdata + 4);
+          sprintf (buf, ".module/%08lx", (unsigned long) base_addr);
+        }
+      else
+        {
+          base_addr = bfd_get_64 (abfd, note->descdata + 4);
+          sprintf (buf, ".module/%016lx", (unsigned long) base_addr);
+        }
 
       len = strlen (buf) + 1;
       name = (char *) bfd_alloc (abfd, len);
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 9dae287554c..7dffad00240 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -1134,8 +1134,10 @@  core_process_module_section (bfd *abfd, asection *sect, void *obj)
   struct cpms_data *data = (struct cpms_data *) obj;
   enum bfd_endian byte_order = gdbarch_byte_order (data->gdbarch);
 
+  unsigned int data_type;
   char *module_name;
   size_t module_name_size;
+  size_t module_name_offset;
   CORE_ADDR base_addr;
 
   gdb_byte *buf = NULL;
@@ -1156,16 +1158,24 @@  core_process_module_section (bfd *abfd, asection *sect, void *obj)
 
 
   /* A DWORD (data_type) followed by struct windows_core_module_info.  */
+  data_type = extract_unsigned_integer (buf, 4, byte_order);
 
-  base_addr =
-    extract_unsigned_integer (buf + 4, 4, byte_order);
-
-  module_name_size =
-    extract_unsigned_integer (buf + 8, 4, byte_order);
+  if (data_type == 3) /* NOTE_INFO_MODULE */
+    {
+      base_addr = extract_unsigned_integer (buf + 4, 4, byte_order);
+      module_name_size = extract_unsigned_integer (buf + 8, 4, byte_order);
+      module_name_offset = 12;
+    }
+  else /* NOTE_INFO_MODULE64 */
+    {
+      base_addr = extract_unsigned_integer (buf + 4, 8, byte_order);
+      module_name_size = extract_unsigned_integer (buf + 12, 4, byte_order);
+      module_name_offset = 16;
+    }
 
-  if (12 + module_name_size > bfd_section_size (sect))
+  if (module_name_offset + module_name_size > bfd_section_size (sect))
     goto out;
-  module_name = (char *) buf + 12;
+  module_name = (char *) buf + module_name_offset;
 
   /* The first module is the .exe itself.  */
   if (data->module_count != 0)