[PATCH v2 0/2] Print out load addresses for .text and .data

[PATCH v2 0/2] Print out load addresses for .text and .data

[Robbie Harwood]

Hi, this is a new iteration of the patches to display virtual addresses of
grub-core/kernel.exec for use in gdb.

For this iteration, I've taken Thomas's suggestion (thanks!) to address
Glenn's reviews: instead of hiding file:line numbers, they are printed on
their own line, commented out.

Be well,
--Robbie

Peter Jones (1):
  Make a "gdb" dprintf that tells us load addresses

Robbie Harwood (1):
  Add gdb-friendly output support to grub_real_dprintf()

 grub-core/kern/dl.c       | 50 +++++++++++++++++++++++++++++++++++++++
 grub-core/kern/efi/efi.c  |  4 ++--
 grub-core/kern/efi/init.c | 26 +++++++++++++++++++-
 grub-core/kern/misc.c     |  7 +++++-
 include/grub/efi/efi.h    |  2 +-
 5 files changed, 84 insertions(+), 5 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] Add gdb-friendly output support to grub_real_dprintf()

[Robbie Harwood]

When condition is gdb, comment out the file/line/system information by
prepending '#' and putting the command on its own line.  This allows
easy copy/pasting of gdb commands into gdb while preserving the
information about where the print came from and how to disable it.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/kern/misc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 11b8592c8..7eea900ae 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -182,7 +182,12 @@ grub_real_dprintf (const char *file, const int line, const char *condition,
 
   if (grub_debug_enabled (condition))
     {
-      grub_printf ("%s:%d:%s: ", file, line, condition);
+      /* gdb commands are on their own line to ease copy/pasting into gdb. */
+      if (!grub_strcmp(condition, "gdb"))
+	grub_printf("#%s:%d:%s:\n", file, line, condition);
+      else
+	grub_printf ("%s:%d:%s: ", file, line, condition);
+
       va_start (args, fmt);
       grub_vprintf (fmt, args);
       va_end (args);
-- 
2.33.0

[PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Robbie Harwood]

From: Peter Jones <pjones@redhat.com>

Add a grub_dprintf() call during platform init and during module loading
that tells us the virtual addresses of the .text and .data sections of
grub-core/kernel.exec and any modules it loads.

Specifically, it displays them in the gdb "add-symbol-file" syntax, with
the presumption that there's a variable $grubdir that reflects the path
to any such binaries.

Signed-off-by: Peter Jones <pjones@redhat.com>
[rharwood@redhat.com: remove custom function, adjust commit message]
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/kern/dl.c       | 50 +++++++++++++++++++++++++++++++++++++++
 grub-core/kern/efi/efi.c  |  4 ++--
 grub-core/kern/efi/init.c | 26 +++++++++++++++++++-
 include/grub/efi/efi.h    |  2 +-
 4 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index 6a52de168..a5def4ea3 100644
--- a/grub-core/kern/dl.c
+++ b/grub-core/kern/dl.c
@@ -447,6 +447,23 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
       return s;
   return NULL;
 }
+static long
+grub_dl_find_section_index (Elf_Ehdr *e, const char *name)
+{
+  Elf_Shdr *s;
+  const char *str;
+  unsigned i;
+
+  s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * e->e_shentsize);
+  str = (char *) e + s->sh_offset;
+
+  for (i = 0, s = (Elf_Shdr *) ((char *) e + e->e_shoff);
+       i < e->e_shnum;
+       i++, s = (Elf_Shdr *) ((char *) s + e->e_shentsize))
+    if (grub_strcmp (str + s->sh_name, name) == 0)
+      return (long)i;
+  return -1;
+}
 
 /* Me, Vladimir Serbinenko, hereby I add this module check as per new
    GNU module policy. Note that this license check is informative only.
@@ -607,6 +624,37 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
 
   return GRUB_ERR_NONE;
 }
+static void
+grub_dl_print_gdb_info (grub_dl_t mod, Elf_Ehdr *e)
+{
+  void *text, *data = NULL;
+  long idx;
+
+  idx = grub_dl_find_section_index (e, ".text");
+  if (idx < 0)
+    return;
+
+  text = grub_dl_get_section_addr (mod, idx);
+  if (!text)
+    return;
+
+  idx = grub_dl_find_section_index (e, ".data");
+  if (idx >= 0)
+    data = grub_dl_get_section_addr (mod, idx);
+
+  if (data)
+    grub_dprintf ("gdb", "add-symbol-file \\\n"
+		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
+		  "\\\n %p -s .data %p\n",
+		  GRUB_TARGET_CPU, GRUB_PLATFORM,
+		  mod->name, text, data);
+  else
+    grub_dprintf ("gdb", "add-symbol-file \\\n"
+		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
+		  "\\\n%p\n",
+		  GRUB_TARGET_CPU, GRUB_PLATFORM,
+		  mod->name, text);
+}
 
 /* Load a module from core memory.  */
 grub_dl_t
@@ -666,6 +714,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
   grub_dprintf ("modules", "module name: %s\n", mod->name);
   grub_dprintf ("modules", "init function: %p\n", mod->init);
 
+  grub_dl_print_gdb_info (mod, e);
+
   if (grub_dl_add (mod))
     {
       grub_dl_unload (mod);
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 8cff7be02..f12fd5893 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -291,7 +291,7 @@ grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
 /* Search the mods section from the PE32/PE32+ image. This code uses
    a PE32 header, but should work with PE32+ as well.  */
 grub_addr_t
-grub_efi_modules_addr (void)
+grub_efi_section_addr (const char *section_name)
 {
   grub_efi_loaded_image_t *image;
   struct grub_pe32_header *header;
@@ -316,7 +316,7 @@ grub_efi_modules_addr (void)
        i < coff_header->num_sections;
        i++, section++)
     {
-      if (grub_strcmp (section->name, "mods") == 0)
+      if (grub_strcmp (section->name, section_name) == 0)
 	break;
     }
 
diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 7facacf09..35f787915 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -82,10 +82,33 @@ stack_protector_init (void)
 
 grub_addr_t grub_modbase;
 
+static void
+grub_efi_print_gdb_info (void)
+{
+  grub_addr_t text;
+  grub_addr_t data;
+
+  text = grub_efi_section_addr (".text");
+  if (!text)
+    return;
+
+  data = grub_efi_section_addr (".data");
+  if (data)
+    grub_dprintf ("gdb",
+		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
+		  "kernel.exec %p -s .data %p\n",
+		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text, (void *)data);
+  else
+    grub_dprintf ("gdb",
+		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
+		  "kernel.exec %p\n",
+		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text);
+}
+
 void
 grub_efi_init (void)
 {
-  grub_modbase = grub_efi_modules_addr ();
+  grub_modbase = grub_efi_section_addr ("mods");
   /* First of all, initialize the console so that GRUB can display
      messages.  */
   grub_console_init ();
@@ -108,6 +131,7 @@ grub_efi_init (void)
   efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
 	      0, 0, 0, NULL);
 
+  grub_efi_print_gdb_info ();
   grub_efidisk_init ();
 }
 
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 83d958f99..73b754177 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -105,7 +105,7 @@ grub_err_t grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
                                            char *args);
 #endif
 
-grub_addr_t grub_efi_modules_addr (void);
+grub_addr_t grub_efi_section_addr (const char *section);
 
 void grub_efi_mm_init (void);
 void grub_efi_mm_fini (void);
-- 
2.33.0

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Daniel Kiper]

On Wed, Nov 03, 2021 at 02:22:07PM -0400, Robbie Harwood wrote:
> From: Peter Jones <pjones@redhat.com>
>
> Add a grub_dprintf() call during platform init and during module loading
> that tells us the virtual addresses of the .text and .data sections of
> grub-core/kernel.exec and any modules it loads.
>
> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> the presumption that there's a variable $grubdir that reflects the path
> to any such binaries.

Could you tell us why this thing has to be displayed as a part of debug
messages? Could you create a separate command which would do the same?

Additionally, I think this kind of information should not be printed if
the lockdown/UEFI Secure Boot is enforced.

> Signed-off-by: Peter Jones <pjones@redhat.com>
> [rharwood@redhat.com: remove custom function, adjust commit message]
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/kern/dl.c       | 50 +++++++++++++++++++++++++++++++++++++++
>  grub-core/kern/efi/efi.c  |  4 ++--
>  grub-core/kern/efi/init.c | 26 +++++++++++++++++++-
>  include/grub/efi/efi.h    |  2 +-
>  4 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 6a52de168..a5def4ea3 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -447,6 +447,23 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
>        return s;
>    return NULL;
>  }
> +static long
> +grub_dl_find_section_index (Elf_Ehdr *e, const char *name)
> +{
> +  Elf_Shdr *s;
> +  const char *str;
> +  unsigned i;
> +
> +  s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * e->e_shentsize);
> +  str = (char *) e + s->sh_offset;
> +
> +  for (i = 0, s = (Elf_Shdr *) ((char *) e + e->e_shoff);
> +       i < e->e_shnum;
> +       i++, s = (Elf_Shdr *) ((char *) s + e->e_shentsize))
> +    if (grub_strcmp (str + s->sh_name, name) == 0)
> +      return (long)i;
> +  return -1;
> +}
>
>  /* Me, Vladimir Serbinenko, hereby I add this module check as per new
>     GNU module policy. Note that this license check is informative only.
> @@ -607,6 +624,37 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
>
>    return GRUB_ERR_NONE;
>  }
> +static void
> +grub_dl_print_gdb_info (grub_dl_t mod, Elf_Ehdr *e)
> +{
> +  void *text, *data = NULL;
> +  long idx;
> +
> +  idx = grub_dl_find_section_index (e, ".text");
> +  if (idx < 0)
> +    return;
> +
> +  text = grub_dl_get_section_addr (mod, idx);
> +  if (!text)
> +    return;
> +
> +  idx = grub_dl_find_section_index (e, ".data");
> +  if (idx >= 0)
> +    data = grub_dl_get_section_addr (mod, idx);
> +
> +  if (data)
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
> +		  "\\\n %p -s .data %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM,
> +		  mod->name, text, data);
> +  else
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "

No, paths cannot be hard coded like here. At least part of them should be
taken from the configure.

Daniel

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Robbie Harwood]

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

Daniel Kiper <dkiper@net-space.pl> writes:

> On Wed, Nov 03, 2021 at 02:22:07PM -0400, Robbie Harwood wrote:
>> From: Peter Jones <pjones@redhat.com>
>>
>> Add a grub_dprintf() call during platform init and during module loading
>> that tells us the virtual addresses of the .text and .data sections of
>> grub-core/kernel.exec and any modules it loads.
>>
>> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
>> the presumption that there's a variable $grubdir that reflects the path
>> to any such binaries.
>
> Could you tell us why this thing has to be displayed as a part of debug
> messages? Could you create a separate command which would do the same?

I don't follow what you're suggesting.  It's debug output and gdb is a
debugger.  What would you have it do instead?

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Daniel Kiper]

On Mon, Nov 29, 2021 at 06:11:36PM -0500, Robbie Harwood wrote:
> Daniel Kiper <dkiper@net-space.pl> writes:
>
> > On Wed, Nov 03, 2021 at 02:22:07PM -0400, Robbie Harwood wrote:
> >> From: Peter Jones <pjones@redhat.com>
> >>
> >> Add a grub_dprintf() call during platform init and during module loading
> >> that tells us the virtual addresses of the .text and .data sections of
> >> grub-core/kernel.exec and any modules it loads.
> >>
> >> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> >> the presumption that there's a variable $grubdir that reflects the path
> >> to any such binaries.
> >
> > Could you tell us why this thing has to be displayed as a part of debug
> > messages? Could you create a separate command which would do the same?
>
> I don't follow what you're suggesting.  It's debug output and gdb is a
> debugger.  What would you have it do instead?

I cannot see any reason of displaying this kind of stuff as a part of
debug messages. I think we should have a separate command, e.g.
give_me_these_load_addresses ;-), in the GRUB which will produce list
of commands needed to be executed in the gdb.

Daniel

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Peter Jones]

On Tue, Nov 30, 2021 at 05:08:20PM +0100, Daniel Kiper wrote:
> On Mon, Nov 29, 2021 at 06:11:36PM -0500, Robbie Harwood wrote:
> > Daniel Kiper <dkiper@net-space.pl> writes:
> >
> > > On Wed, Nov 03, 2021 at 02:22:07PM -0400, Robbie Harwood wrote:
> > >> From: Peter Jones <pjones@redhat.com>
> > >>
> > >> Add a grub_dprintf() call during platform init and during module loading
> > >> that tells us the virtual addresses of the .text and .data sections of
> > >> grub-core/kernel.exec and any modules it loads.
> > >>
> > >> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> > >> the presumption that there's a variable $grubdir that reflects the path
> > >> to any such binaries.
> > >
> > > Could you tell us why this thing has to be displayed as a part of debug
> > > messages? Could you create a separate command which would do the same?
> >
> > I don't follow what you're suggesting.  It's debug output and gdb is a
> > debugger.  What would you have it do instead?
> 
> I cannot see any reason of displaying this kind of stuff as a part of
> debug messages. I think we should have a separate command, e.g.
> give_me_these_load_addresses ;-), in the GRUB which will produce list
> of commands needed to be executed in the gdb.

I'm not seeing how that will fulfill the goals.  Typically when I need
gdb in grub, it's well before /commands/ actually work at all.

I definitely *do* see your point about silencing this when SB is
enabled; that's useful feedback.

-- 
        Peter

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Daniel Kiper]

On Wed, Dec 01, 2021 at 11:08:39AM -0500, Peter Jones wrote:
> On Tue, Nov 30, 2021 at 05:08:20PM +0100, Daniel Kiper wrote:
> > On Mon, Nov 29, 2021 at 06:11:36PM -0500, Robbie Harwood wrote:
> > > Daniel Kiper <dkiper@net-space.pl> writes:
> > >
> > > > On Wed, Nov 03, 2021 at 02:22:07PM -0400, Robbie Harwood wrote:
> > > >> From: Peter Jones <pjones@redhat.com>
> > > >>
> > > >> Add a grub_dprintf() call during platform init and during module loading
> > > >> that tells us the virtual addresses of the .text and .data sections of
> > > >> grub-core/kernel.exec and any modules it loads.
> > > >>
> > > >> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> > > >> the presumption that there's a variable $grubdir that reflects the path
> > > >> to any such binaries.
> > > >
> > > > Could you tell us why this thing has to be displayed as a part of debug
> > > > messages? Could you create a separate command which would do the same?
> > >
> > > I don't follow what you're suggesting.  It's debug output and gdb is a
> > > debugger.  What would you have it do instead?
> >
> > I cannot see any reason of displaying this kind of stuff as a part of
> > debug messages. I think we should have a separate command, e.g.
> > give_me_these_load_addresses ;-), in the GRUB which will produce list
> > of commands needed to be executed in the gdb.
>
> I'm not seeing how that will fulfill the goals.  Typically when I need
> gdb in grub, it's well before /commands/ actually work at all.

OK, makes sense for me. However, how do you enable debugging then? Do you
use an early GRUB cfg? If yes why could not we use command instead of
setting the variable there? Hmmm... I can only see one advantage of your
solution over mine. A gdb command is printed and available immediately
after loading a module into the memory. Disadvantage, small one?, is
that it is mixed with the other debug messages.

> I definitely *do* see your point about silencing this when SB is
> enabled; that's useful feedback.

Great! Thanks!

Daniel

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Glenn Washburn]

On Wed,  3 Nov 2021 14:22:07 -0400
Robbie Harwood <rharwood@redhat.com> wrote:

> From: Peter Jones <pjones@redhat.com>
> 
> Add a grub_dprintf() call during platform init and during module loading
> that tells us the virtual addresses of the .text and .data sections of
> grub-core/kernel.exec and any modules it loads.
> 
> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> the presumption that there's a variable $grubdir that reflects the path
> to any such binaries.

I don't think this patch has the $grubdir that's mentioned here. I
think this is useful information that is output in this patch, but I
would prefer a different mechanism. How about printing normally (ie use
grub_printf instead of grub_dprintf), have the code conditionally
compiled in if a certain macro is defined.

So here have something like...

> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> [rharwood@redhat.com: remove custom function, adjust commit message]
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/kern/dl.c       | 50 +++++++++++++++++++++++++++++++++++++++
>  grub-core/kern/efi/efi.c  |  4 ++--
>  grub-core/kern/efi/init.c | 26 +++++++++++++++++++-
>  include/grub/efi/efi.h    |  2 +-
>  4 files changed, 78 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 6a52de168..a5def4ea3 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -447,6 +447,23 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
>        return s;
>    return NULL;
>  }
> +static long
> +grub_dl_find_section_index (Elf_Ehdr *e, const char *name)
> +{
> +  Elf_Shdr *s;
> +  const char *str;
> +  unsigned i;
> +
> +  s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * e->e_shentsize);
> +  str = (char *) e + s->sh_offset;
> +
> +  for (i = 0, s = (Elf_Shdr *) ((char *) e + e->e_shoff);
> +       i < e->e_shnum;
> +       i++, s = (Elf_Shdr *) ((char *) s + e->e_shentsize))
> +    if (grub_strcmp (str + s->sh_name, name) == 0)
> +      return (long)i;
> +  return -1;
> +}
>  
>  /* Me, Vladimir Serbinenko, hereby I add this module check as per new
>     GNU module policy. Note that this license check is informative only.
> @@ -607,6 +624,37 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
>  
>    return GRUB_ERR_NONE;
>  }

#ifdef PRINT_GDB_SYM_LOAD_CMD

> +static void
> +grub_dl_print_gdb_info (grub_dl_t mod, Elf_Ehdr *e)
> +{
> +  void *text, *data = NULL;
> +  long idx;
> +
> +  idx = grub_dl_find_section_index (e, ".text");
> +  if (idx < 0)
> +    return;
> +
> +  text = grub_dl_get_section_addr (mod, idx);
> +  if (!text)
> +    return;
> +
> +  idx = grub_dl_find_section_index (e, ".data");
> +  if (idx >= 0)
> +    data = grub_dl_get_section_addr (mod, idx);
> +
> +  if (data)
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
> +		  "\\\n %p -s .data %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM,
> +		  mod->name, text, data);
> +  else
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
> +		  "\\\n%p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM,
> +		  mod->name, text);
> +}

#endif

>  
>  /* Load a module from core memory.  */
>  grub_dl_t
> @@ -666,6 +714,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>    grub_dprintf ("modules", "module name: %s\n", mod->name);
>    grub_dprintf ("modules", "init function: %p\n", mod->init);
>  

#ifdef PRINT_GDB_SYM_LOAD_CMD

> +  grub_dl_print_gdb_info (mod, e);

#endif

> +
>    if (grub_dl_add (mod))
>      {
>        grub_dl_unload (mod);
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 8cff7be02..f12fd5893 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -291,7 +291,7 @@ grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
>  /* Search the mods section from the PE32/PE32+ image. This code uses
>     a PE32 header, but should work with PE32+ as well.  */
>  grub_addr_t
> -grub_efi_modules_addr (void)
> +grub_efi_section_addr (const char *section_name)
>  {
>    grub_efi_loaded_image_t *image;
>    struct grub_pe32_header *header;
> @@ -316,7 +316,7 @@ grub_efi_modules_addr (void)
>         i < coff_header->num_sections;
>         i++, section++)
>      {
> -      if (grub_strcmp (section->name, "mods") == 0)
> +      if (grub_strcmp (section->name, section_name) == 0)
>  	break;
>      }
>  
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 7facacf09..35f787915 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -82,10 +82,33 @@ stack_protector_init (void)
>  
>  grub_addr_t grub_modbase;
>  

#ifdef PRINT_GDB_SYM_LOAD_CMD

> +static void
> +grub_efi_print_gdb_info (void)
> +{
> +  grub_addr_t text;
> +  grub_addr_t data;
> +
> +  text = grub_efi_section_addr (".text");
> +  if (!text)
> +    return;
> +
> +  data = grub_efi_section_addr (".data");
> +  if (data)
> +    grub_dprintf ("gdb",
> +		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
> +		  "kernel.exec %p -s .data %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text, (void *)data);
> +  else
> +    grub_dprintf ("gdb",
> +		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
> +		  "kernel.exec %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text);
> +}

#endif

> +
>  void
>  grub_efi_init (void)
>  {
> -  grub_modbase = grub_efi_modules_addr ();
> +  grub_modbase = grub_efi_section_addr ("mods");
>    /* First of all, initialize the console so that GRUB can display
>       messages.  */
>    grub_console_init ();
> @@ -108,6 +131,7 @@ grub_efi_init (void)
>    efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
>  	      0, 0, 0, NULL);
>  

#ifdef PRINT_GDB_SYM_LOAD_CMD

> +  grub_efi_print_gdb_info ();

#endif

>    grub_efidisk_init ();
>  }
>  
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 83d958f99..73b754177 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -105,7 +105,7 @@ grub_err_t grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
>                                             char *args);
>  #endif
>  
> -grub_addr_t grub_efi_modules_addr (void);
> +grub_addr_t grub_efi_section_addr (const char *section);
>  
>  void grub_efi_mm_init (void);
>  void grub_efi_mm_fini (void);

And then in config.h.in have something like:

/* Define to 1 to enable printing of gdb command to load module symbols.  */
#ifdef PRINT_GDB_SYM_LOAD_CMD 0

This should get rid of the need to modify the grub_real_dprintf in the
previous patch. Would this satisfy you use case?

Glenn

Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses

[Glenn Washburn]

On Wed,  3 Nov 2021 14:22:07 -0400
Robbie Harwood <rharwood@redhat.com> wrote:

> From: Peter Jones <pjones@redhat.com>
> 
> Add a grub_dprintf() call during platform init and during module loading
> that tells us the virtual addresses of the .text and .data sections of
> grub-core/kernel.exec and any modules it loads.
> 
> Specifically, it displays them in the gdb "add-symbol-file" syntax, with
> the presumption that there's a variable $grubdir that reflects the path
> to any such binaries.
> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> [rharwood@redhat.com: remove custom function, adjust commit message]
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/kern/dl.c       | 50 +++++++++++++++++++++++++++++++++++++++
>  grub-core/kern/efi/efi.c  |  4 ++--
>  grub-core/kern/efi/init.c | 26 +++++++++++++++++++-
>  include/grub/efi/efi.h    |  2 +-
>  4 files changed, 78 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 6a52de168..a5def4ea3 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -447,6 +447,23 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
>        return s;
>    return NULL;
>  }
> +static long
> +grub_dl_find_section_index (Elf_Ehdr *e, const char *name)
> +{
> +  Elf_Shdr *s;
> +  const char *str;
> +  unsigned i;
> +
> +  s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * e->e_shentsize);
> +  str = (char *) e + s->sh_offset;
> +
> +  for (i = 0, s = (Elf_Shdr *) ((char *) e + e->e_shoff);
> +       i < e->e_shnum;
> +       i++, s = (Elf_Shdr *) ((char *) s + e->e_shentsize))
> +    if (grub_strcmp (str + s->sh_name, name) == 0)
> +      return (long)i;
> +  return -1;
> +}
>  
>  /* Me, Vladimir Serbinenko, hereby I add this module check as per new
>     GNU module policy. Note that this license check is informative only.
> @@ -607,6 +624,37 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
>  
>    return GRUB_ERR_NONE;
>  }
> +static void
> +grub_dl_print_gdb_info (grub_dl_t mod, Elf_Ehdr *e)
> +{
> +  void *text, *data = NULL;
> +  long idx;
> +
> +  idx = grub_dl_find_section_index (e, ".text");
> +  if (idx < 0)
> +    return;
> +
> +  text = grub_dl_get_section_addr (mod, idx);
> +  if (!text)
> +    return;
> +
> +  idx = grub_dl_find_section_index (e, ".data");
> +  if (idx >= 0)
> +    data = grub_dl_get_section_addr (mod, idx);
> +
> +  if (data)
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
> +		  "\\\n %p -s .data %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM,
> +		  mod->name, text, data);
> +  else
> +    grub_dprintf ("gdb", "add-symbol-file \\\n"
> +		  "/usr/lib/debug/usr/lib/grub/%s-%s/%s.debug "
> +		  "\\\n%p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM,
> +		  mod->name, text);
> +}
>  
>  /* Load a module from core memory.  */
>  grub_dl_t
> @@ -666,6 +714,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>    grub_dprintf ("modules", "module name: %s\n", mod->name);
>    grub_dprintf ("modules", "init function: %p\n", mod->init);
>  
> +  grub_dl_print_gdb_info (mod, e);
> +
>    if (grub_dl_add (mod))
>      {
>        grub_dl_unload (mod);
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 8cff7be02..f12fd5893 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -291,7 +291,7 @@ grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
>  /* Search the mods section from the PE32/PE32+ image. This code uses
>     a PE32 header, but should work with PE32+ as well.  */
>  grub_addr_t
> -grub_efi_modules_addr (void)
> +grub_efi_section_addr (const char *section_name)
>  {
>    grub_efi_loaded_image_t *image;
>    struct grub_pe32_header *header;
> @@ -316,7 +316,7 @@ grub_efi_modules_addr (void)
>         i < coff_header->num_sections;
>         i++, section++)
>      {
> -      if (grub_strcmp (section->name, "mods") == 0)
> +      if (grub_strcmp (section->name, section_name) == 0)
>  	break;
>      }
>  
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 7facacf09..35f787915 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -82,10 +82,33 @@ stack_protector_init (void)
>  
>  grub_addr_t grub_modbase;
>  
> +static void
> +grub_efi_print_gdb_info (void)
> +{
> +  grub_addr_t text;
> +  grub_addr_t data;
> +
> +  text = grub_efi_section_addr (".text");
> +  if (!text)
> +    return;
> +
> +  data = grub_efi_section_addr (".data");
> +  if (data)
> +    grub_dprintf ("gdb",
> +		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
> +		  "kernel.exec %p -s .data %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text, (void *)data);

As far as I can tell, this is an incorrect value for data. The value
for the data segment that gets displayed is the location of the UEFI
data segment (PE32+ format). But the location you want is of the ELF
data segment in kernel.exec. The UEFI data segment is a combination of
all the segments marked as data followed by bss from the ELF kernel.exec
file (rodata, rodata.str1.1, data, module_license, and bss from what
I'm seeing).

Does your patch actually work for you? Function symbols will be fine,
but any data variables should be not right (eg. grub_dl_head).

Currently, GRUB doesn't have enough information to reconstruct where
ELF data segments and BSS map into the PE32+ data segment. It needs the
name, size and alignment of the ELF segments to figure it out.

I see a couple solutions.
1. Have a function in gdb_grub that figures everything out based on the
   PE32+ data segment offset. I could use objdump to get the info from
   kernel.exec and then load the symbol file itself. It would be called
   like "grub_add_symbols <text offset> <data offset>".
2. Have grub-mkimage insert the required data into the UEFI binary so
   GRUB can figure it out and print the add-symbol-file line as you're
   wanting to do in this patch. Perhaps it could go into a new section
   in the UEFI binary or patch it into a global variable (I'd prefer the
   former).

Option 1 seems more hackish, but probably easier (I've got a working
POC). Option 2 using another PE32+ section, seems like a cleaner
approach, but (maybe) more work.

Glenn

> +  else
> +    grub_dprintf ("gdb",
> +		  "add-symbol-file /usr/lib/debug/usr/lib/grub/%s-%s/"
> +		  "kernel.exec %p\n",
> +		  GRUB_TARGET_CPU, GRUB_PLATFORM, (void *)text);
> +}
> +
>  void
>  grub_efi_init (void)
>  {
> -  grub_modbase = grub_efi_modules_addr ();
> +  grub_modbase = grub_efi_section_addr ("mods");
>    /* First of all, initialize the console so that GRUB can display
>       messages.  */
>    grub_console_init ();
> @@ -108,6 +131,7 @@ grub_efi_init (void)
>    efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
>  	      0, 0, 0, NULL);
>  
> +  grub_efi_print_gdb_info ();
>    grub_efidisk_init ();
>  }
>  
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 83d958f99..73b754177 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -105,7 +105,7 @@ grub_err_t grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
>                                             char *args);
>  #endif
>  
> -grub_addr_t grub_efi_modules_addr (void);
> +grub_addr_t grub_efi_section_addr (const char *section);
>  
>  void grub_efi_mm_init (void);
>  void grub_efi_mm_fini (void);