grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Glenn Washburn
Subject: Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
Date: Wed, 26 Jan 2022 02:02:45 -0600

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]