[PATCH 0/8] GDB script fixes and improvements

[PATCH 0/8] GDB script fixes and improvements

[Glenn Washburn]

This is a collection of fixes and improvements mostly to the GDB script,
gdb_grub. I don't think the first 4 need commnt.

A major benefit added in patch #5 is the ability to properly load symbols on
EFI targets given the load address for the text and data segments. This is
written as a shell script in the GDB script primarily because I'm not
familiar with the Python API. I think it would probably be better written in
Python, though. Daniel what do you think about using Python in the GDB script?
First of all, I get the impression that this script isn't used much (but I'd
like to know if I'm wrong about that), so I suspect changes won't affect many
people one way or the other. And second, I think its reasonable to assume
these days that people's GDB is compiled with Python support.

Patch #6 has code stolen from the patch by Peter Jones[1]. I've added a
Co-developed-by tag. Let me know if there's something different I should do.
I've removed the printing of lines for each module as this is unnecessary if
the kernel.exec symbols have been loaded correctly (see load_all_modules in
gdb_grub). Also, I've made the printed message for loading kernel.exec
symbols only happen if enabled by setting PRINT_GDB_SYM_LOAD_CMD to a non-
zero value in config.h.in.

Ultimately, I think a better solution is to check the EFI variables for a
certain value and enable the print using that. This way support would always
be compiled in and can still be enabled very early in the GRUB boot process.
However, I'm not sure what that variable should be, or if we should just
create our own variable. Someone more experienced in EFI than I could help
with that or say that using an EFI var would be unadvisable.

Testing on QEMU with OVMF firmware, shows that GRUB will be loaded to the
same address consistently so long as the binary hasn't changed. This is nice
because by the time you get the message with the right parameters needed to
load the symbols you may have already passed by code you wanted to break on.
So you can use the same parameters to load the symbols before GRUB executes
and set break points as early as you want in GRUB code. I'm not sure if this
is true of real hardware, but I suspect it is. If this is so, then Daniel's
idea of making this a command may make sense as well, so that GRUB doesn't
need to be recompiled in "debug mode" to get this address. Of course, if
GRUB is failing before module initialization, the module won't work.

Patch #7 adds a check if the build was for an EFI platform and runs the
dynamic code at startup if so. As noted in the commit message, without this
the user must remember to first remove the symbols loaded at the statically
positioned offset. Otherwise, symbols will be multiply defined and GDB uses
the first ones defined by default. It was suggested by Vladimir that targets
with platform as EFI were all the dynamic ones[2], but 10+ years later this
may have changed. Other target tests can be added as needed for ones that are
dynamic but are not EFI.

Patch #8 This is a work around for a strange issue where GDB is stopping
before the stack frame has been setup when breaking on grub_dl_add(). But
GDB thinks that the stack frame has been setup. I'm not sure what is causing
this (not using -fomit-frame-pointer and don't see any obvious culprits in
other GCC compile options). I also don't particularly like this solution,
but its the best one I've found. I originally was going up a frame in
grub_dl_add() and getting the mod variable there, but that assumes knowledge
of the caller. I'd like to set a break point on the instruction that I'd get
to by issuing a "step" in GDB after the break on grub_dl_add, but I don't see
a way to do that. Another option is to set an unused label in grub_dl_add()
and do a tbreak on the label, but I was wanting to avoid code changes.
Maybe this is acceptable though.

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2021-11/msg00008.html
[2] https://lists.gnu.org/archive/html/grub-devel/2011-11/msg00069.html

Glenn Washburn (8):
  gdb: Move runtime module loading into runtime_load_module
  gdb: If no modules have been loaded, do not try to load module symbols
  gdb: Do not lazy load module symbols
  gdb: Prevent wrapping when writing to .segments.tmp
  gdb: Add functions to make loading from dynamically positioned targets
    easier
  gdb: If enabled, print line used to load EFI kernel symbols when using
    gdb_grub script
  gdb: Conditionally run GDB script logic for dynamically or statically
    positioned GRUB
  gdb: Get correct mod variable value

 config.h.in               |   3 ++
 grub-core/gdb_grub.in     | 109 ++++++++++++++++++++++++++++++++++----
 grub-core/gmodule.pl.in   |   2 +-
 grub-core/kern/efi/efi.c  |   4 +-
 grub-core/kern/efi/init.c |  25 ++++++++-
 include/grub/efi/efi.h    |   2 +-
 6 files changed, 131 insertions(+), 14 deletions(-)

-- 
2.27.0

[PATCH 1/8] gdb: Move runtime module loading into runtime_load_module

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index e322d3dc1..b0e8d1ed1 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -70,16 +70,22 @@ document load_all_modules
 	Load debugging information for all loaded modules.
 end
 
+define runtime_load_module
+	break grub_dl_add
+	commands
+		silent
+		load_module mod
+		cont
+	end
+end
+document runtime_load_module
+	Load module symbols at runtime as they are loaded.
+end
+
 ###
 
 set confirm off
 file kernel.exec
 target remote :1234
 
-# inform when module is loaded
-break grub_dl_add
-commands
-	silent
-	load_module mod
-	cont
-end
+runtime_load_module
-- 
2.27.0

[PATCH 2/8] gdb: If no modules have been loaded, do not try to load module symbols

[Glenn Washburn]

This prevents load_all_modules from failing when called before any modules
have been loaded. Failures in GDB user-defined functions cause any function
which called them to also fail.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index b0e8d1ed1..3a0593610 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -64,7 +64,9 @@ define load_all_modules
 		dump_module_sections $this
 		set $this = $this->next
 	end
-	match_and_load_symbols
+	if (grub_dl_head != 0)
+		match_and_load_symbols
+	end
 end
 document load_all_modules
 	Load debugging information for all loaded modules.
-- 
2.27.0

[PATCH 3/8] gdb: Do not lazy load module symbols

[Glenn Washburn]

When loading module symbols, tell GDB to load them all right away. GDB by
default will lazy load symbols as needed in the background. However, this
process will output to GDB's stdout some messages. This fixes a bug where
output from the loading process gets printed to the .segments.tmp file
causing gmodule.pl to write a bad .loadsym.gdb that GDB chokes on.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gmodule.pl.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/gmodule.pl.in b/grub-core/gmodule.pl.in
index 78aa1e64e..c74eedf4a 100644
--- a/grub-core/gmodule.pl.in
+++ b/grub-core/gmodule.pl.in
@@ -11,7 +11,7 @@ use strict;
 while (<>) {
 	my ($name, %sections) = split;
 
-	print "add-symbol-file $name.module";
+	print "add-symbol-file -readnow $name.module";
 
 	open (READELF, "readelf -S $name.mod |") or die;
 	while (<READELF>) {
-- 
2.27.0

[PATCH 4/8] gdb: Prevent wrapping when writing to .segments.tmp

[Glenn Washburn]

GDB logging is redirected to write .segments.tmp, which means that GDB will
wrap lines longer than what it thinks is the screen width (typically 80
characters). When wrapping does occur it causes gmodule.pl to misbehave. So
disable line wrapping.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index 3a0593610..dd1e86bf2 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -13,6 +13,10 @@
 define dump_module_sections
 	set $mod = $arg0
 
+	# Set unlimited width so that lines don't get wrapped writing
+	# to .segments.tmp
+	set width 0
+
 	# FIXME: save logging status
 	set logging file .segments.tmp
 	set logging redirect on
-- 
2.27.0

[PATCH 5/8] gdb: Add functions to make loading from dynamically positioned targets easier

[Glenn Washburn]

Many targets, such as EFI, load GRUB at addresses that are determined at
runtime. So the load addresses in kernel.exec will almost certainly be
wrong. Given the address of the start of the text and data segments, these
functions will tell GDB to load the symbols at the proper locations.
It is left up to the user to determine how to get the test and data
addresses.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 54 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index dd1e86bf2..f3c6faf94 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -9,6 +9,60 @@
 ### Lubomir Kundrak <lkudrak@skosi.org>
 ###
 
+define dynamic_load_kernel_exec_symbols
+	shell rm -f .remove-kernel.exec.symfile.gdb
+
+	# Loading symbols is complicated by the fact that kernel.exec is an ELF
+	# ELF binary, but the UEFI runtime is PE32+. All the data sections of
+	# the ELF binary are concatenated (accounting for ELF section alignment)
+	# and put into one .data section in the PE32+ runtime image. So given
+	# the load address of the .data PE32+ section we can determine the
+	# addresses each ELF data section maps to.
+	shell bash -c ' \
+	( \
+	  export PE_TEXT=$1 PE_DATA=$2; \
+	  alignup() { \
+	    PAD=1; \
+	    if [ "$(($1%$2))" -eq 0 ]; then \
+	      PAD=0; \
+	    fi; \
+	    printf "0x%x\n" $(((($1/$2)+$PAD)*$2)); \
+	  }; \
+	  printf "add-symbol-file kernel.exec ${PE_TEXT}"; \
+	  objdump -h kernel.exec | tail -n +6 | \
+	    while read IDX NAME SIZE _ _ OFFSET ALIGN; do \
+	      read FLAGS; \
+	      if [ -n "$FLAGS" ] && [ -z "${FLAGS%%*DATA*}" -o "$NAME" = .bss ]; then \
+		OFF=$(alignup ${OFF:-0} $ALIGN); \
+		echo -n " -s $NAME (${PE_DATA}+$(printf "0x%x" $OFF))"; \
+		OFF=$((${OFF} + 0x${SIZE})); \
+	      fi; \
+	    done \
+	)' script $arg0 $arg1 >.kernel.exec.loadsym.gdb
+	source .kernel.exec.loadsym.gdb
+end
+document dynamic_load_kernel_exec_symbols
+	Load debugging symbols from kernel.exec given the address of the
+	.text and .data segments of the UEFI binary.
+end
+
+define dynamic_load_symbols
+	dynamic_load_kernel_exec_symbols $arg0 $arg1
+
+	# We may have been very late to loading the kernel.exec symbols and
+	# and modules may already be loaded. So load symbols for any already
+	# loaded.
+	load_all_modules
+
+	runtime_load_module
+end
+document dynamic_load_symbols
+	Load debugging symbols from kernel.exec and any loaded modules given
+	the address of the .text and .data segments of the UEFI binary. Also
+	setup session to automatically load module symbols for modules loaded
+	in the future.
+end
+
 # Add section numbers and addresses to .segments.tmp
 define dump_module_sections
 	set $mod = $arg0
-- 
2.27.0

[PATCH 6/8] gdb: If enabled, print line used to load EFI kernel symbols when using gdb_grub script

[Glenn Washburn]

If the macro PRINT_GDB_SYM_LOAD_CMD is non-zero, compile code which will
print the command needed to load symbols for the GRUB EFI kernel. This is
needed because EFI firmware determines where to load the GRUB EFI
at runtime, and so the relevant addresses are not known ahead of time.

The command is a custom command defined in the gdb_grub GDB script. So GDB
should be started with the script as an argument to the -x option or
sourced into an active GDB session before running the outputted command.

Co-developed-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 config.h.in               |  3 +++
 grub-core/kern/efi/efi.c  |  4 ++--
 grub-core/kern/efi/init.c | 25 ++++++++++++++++++++++++-
 include/grub/efi/efi.h    |  2 +-
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/config.h.in b/config.h.in
index 9e8f9911b..6d0743a78 100644
--- a/config.h.in
+++ b/config.h.in
@@ -9,6 +9,9 @@
 #define GCRYPT_NO_DEPRECATED 1
 #define HAVE_MEMMOVE 1
 
+/* Define to 1 to enable printing of gdb command to load module symbols.  */
+#define PRINT_GDB_SYM_LOAD_CMD 0
+
 /* Define to 1 to enable disk cache statistics.  */
 #define DISK_CACHE_STATS @DISK_CACHE_STATS@
 #define BOOT_TIME_STATS @BOOT_TIME_STATS@
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 18858c327..f492cdd99 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -299,7 +299,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;
@@ -324,7 +324,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..f640b8192 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -82,10 +82,30 @@ stack_protector_init (void)
 
 grub_addr_t grub_modbase;
 
+#if 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_printf ("dynamic_load_symbols %p -s .data %p\n",
+		 (void *)text, (void *)data);
+  else
+    grub_printf ("dynamic_load_symbols %p\n", (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 +128,9 @@ grub_efi_init (void)
   efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
 	      0, 0, 0, NULL);
 
+#if 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 fc723962d..f3ed52dda 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -107,7 +107,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.27.0

[PATCH 7/8] gdb: Conditionally run GDB script logic for dynamically or statically positioned GRUB

[Glenn Washburn]

There are broadly two classes of targets to consider when loading symbols
for GRUB, targets that determine where to load GRUB at runtime (dynamically
positioned) and those that do not (statically positioned). For statically
poisitioned targets, symbol loading is determined at link time, so nothing
more needs to be known to load the symbols. For dynamically positioned
targets, such as EFI targets, at runtime symbols should be offset by an
amount that depends on where the runtime chose to load GRUB.

It is important to not load symbols statically for dynamic targets because
then when subsequently loading the symbols correctly one must take care to
remove the existing static symbols, otherwise there will be two sets of
symbols and GDB seems to prefer the ones loaded first (ie the static ones).

Use autoconf variables to generate a gdb_grub for a particular target, which
conditionally run startup code depending on if the target uses static or
dynamic loading. This is complicated by the fact that a subshell is used to
check the autoconf variable and shells return 0 for success, but 0 is false
in conditionals in GDB. So an inversion of the status code is needed.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index f3c6faf94..4c2b8bf52 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -145,7 +145,23 @@ end
 ###
 
 set confirm off
-file kernel.exec
-target remote :1234
 
-runtime_load_module
+# Note: On EFI and other platforms that load GRUB to an address that is
+# determined at runtime, the symbols in kernel.exec will be wrong.
+# However, we must start by loading some executable file or GDB will
+# fail.
+
+# If this returns success, the exit code is 0, however, GDB's "if"
+# considers 0 to be false. So invert the status code.
+shell test "@platform@" == "efi"
+set $platform_efi = ! $_shell_exitcode
+
+if $platform_efi
+	# Only load the executable file, not the symbols
+	exec-file kernel.exec
+else
+	file kernel.exec
+	runtime_load_module
+end
+
+target remote :1234
-- 
2.27.0

[PATCH 8/8] gdb: Get correct mod variable value

[Glenn Washburn]

For some reason, GDB is breaking on grub_dl_add() before the function has
setup its stack frame, but GDB thinks it has. So the value of mod is bogus.
To get the correct value, create a one-time break on grub_dl_get(), which is
the first line of grub_dl_add(). When this break point hits, grub_dl_add()
will have finished setting up it stack frame. But at this point we will be
in grub_dl_get()'s stack frame. So go one frame up, which will be
grub_dl_add(), to get mod's value.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/gdb_grub.in | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index 4c2b8bf52..a1031e58d 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -134,7 +134,16 @@ define runtime_load_module
 	break grub_dl_add
 	commands
 		silent
-		load_module mod
+		# GDB has stopped before the call frame is setup, so mod does
+		# not have the correct value. Create a one-time break on the
+		# next function call and then go one frame up, back to the
+		# grub_dl_add frame, to get the correct value for mod.
+		tbreak grub_dl_get
+		commands
+			fr 1
+			load_module mod
+			cont
+		end
 		cont
 	end
 end
-- 
2.27.0