* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-03 18:22 ` [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses Robbie Harwood
@ 2021-11-25 17:19 ` Daniel Kiper
2021-11-29 23:11 ` Robbie Harwood
2022-01-26 8:02 ` Glenn Washburn
2022-01-27 8:19 ` Glenn Washburn
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Kiper @ 2021-11-25 17:19 UTC (permalink / raw)
To: Robbie Harwood; +Cc: grub-devel, Peter Jones
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-25 17:19 ` Daniel Kiper
@ 2021-11-29 23:11 ` Robbie Harwood
2021-11-30 16:08 ` Daniel Kiper
0 siblings, 1 reply; 10+ messages in thread
From: Robbie Harwood @ 2021-11-29 23:11 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, Peter Jones
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-29 23:11 ` Robbie Harwood
@ 2021-11-30 16:08 ` Daniel Kiper
2021-12-01 16:08 ` Peter Jones
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Kiper @ 2021-11-30 16:08 UTC (permalink / raw)
To: Robbie Harwood; +Cc: grub-devel, Peter Jones
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-30 16:08 ` Daniel Kiper
@ 2021-12-01 16:08 ` Peter Jones
2021-12-01 16:42 ` Daniel Kiper
0 siblings, 1 reply; 10+ messages in thread
From: Peter Jones @ 2021-12-01 16:08 UTC (permalink / raw)
To: Daniel Kiper; +Cc: Robbie Harwood, grub-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-12-01 16:08 ` Peter Jones
@ 2021-12-01 16:42 ` Daniel Kiper
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Kiper @ 2021-12-01 16:42 UTC (permalink / raw)
To: Peter Jones; +Cc: Robbie Harwood, grub-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-03 18:22 ` [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses Robbie Harwood
2021-11-25 17:19 ` Daniel Kiper
@ 2022-01-26 8:02 ` Glenn Washburn
2022-01-27 8:19 ` Glenn Washburn
2 siblings, 0 replies; 10+ messages in thread
From: Glenn Washburn @ 2022-01-26 8:02 UTC (permalink / raw)
To: Robbie Harwood; +Cc: The development of GNU GRUB, Peter Jones
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses
2021-11-03 18:22 ` [PATCH v2 2/2] Make a "gdb" dprintf that tells us load addresses Robbie Harwood
2021-11-25 17:19 ` Daniel Kiper
2022-01-26 8:02 ` Glenn Washburn
@ 2022-01-27 8:19 ` Glenn Washburn
2 siblings, 0 replies; 10+ messages in thread
From: Glenn Washburn @ 2022-01-27 8:19 UTC (permalink / raw)
To: Robbie Harwood; +Cc: The development of GNU GRUB, Peter Jones
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);
^ permalink raw reply [flat|nested] 10+ messages in thread