kgdb's add-kld broken on amd64

[John Baldwin]

On Tuesday 16 September 2008 04:07:46 pm Navdeep Parhar wrote:
> Hello everyone,
> 
> The add-kld command in kgdb does not work as expected on amd64
> (I'm using a recent HEAD, problem may affect others too).  It uses
> the same address for all sections:
> 
> (kgdb) add-kld if_cxgb.ko
> add symbol table from file "/boot/kernel/if_cxgb.ko" at
> 	.text_addr = 0xffffffff81022000
> 	.rodata_addr = 0xffffffff81022000
> 	.rodata.str1.8_addr = 0xffffffff81022000
> 	.rodata.str1.1_addr = 0xffffffff81022000
> 	set_modmetadata_set_addr = 0xffffffff81022000
> 	set_sysctl_set_addr = 0xffffffff81022000
> 	set_sysinit_set_addr = 0xffffffff81022000
> 	set_sysuninit_set_addr = 0xffffffff81022000
> 	.data_addr = 0xffffffff81022000
> 	.bss_addr = 0xffffffff81022000
> (y or n)
> 
> This is not correct.  The .text section's address is OK but the
> others are not.
> 
> The problem seems to be that all amd64 kernel objects have VMA set
> to 0 for all sections.  add_section() in gnu/usr.bin/gdb/kgdb/kld.c
> uses this VMA to adjust the address of the section:
> 
> address = asi->base_addr + bfd_get_section_vma(bfd, sect);
> 
> objdump -h shows that the userland objects on amd64 and all
> objects (kernel + userland) on i386 set VMA.  It is only the
> kernel objects on amd64 that have VMA = 0.  (sample output from
> amd64 and i386 machines appended at the end)
> 
> For the time being I've patched kgdb to consider the file offset
> and not the VMA while calculating the section address.  It seems
> to work but is probably not the right way to fix the problem.
> 
> Any thoughts?

Hmm, I wonder if this is because on amd64 modules are .o's rather than .so's. 
It is.  File offset isn't quite right.  Instead, the way 
sys/kern/link_elf_obj.c works is that it just loads the PROGBITS (text, code, 
etc.) and NOBITS (bss) sections in the order they are in the file and 
concatenates them.  So, the relocation logic in kgdb will need to be updated 
to recognize a .o vs .so and apply that algorithm for .o files.

Actually, what I've done is to replace the home-rolled section relocation 
stuff with the gdb primitives that the solib code in gdb uses.  It works here 
on i386, and hopefully this will fix this as this is how the sharedlibrary 
kld stuff is doing the relocations:

--- //depot/vendor/freebsd/src/gnu/usr.bin/gdb/kgdb/kld.c	2008/04/29 20:41:02
+++ //depot/user/jhb/kgdb/gnu/usr.bin/gdb/kgdb/kld.c	2008/09/17 15:27:25
@@ -37,6 +37,7 @@
 #include <command.h>
 #include <completer.h>
 #include <environ.h>
+#include <exec.h>
 #include <frame-unwind.h>
 #include <inferior.h>
 #include <objfiles.h>
@@ -196,39 +197,14 @@
 	return (0);
 }
 
-struct add_section_info {
-	struct section_addr_info *section_addrs;
-	int sect_index;
-	CORE_ADDR base_addr;
-};
-
-static void
-add_section (bfd *bfd, asection *sect, void *arg)
-{
-	struct add_section_info *asi = arg;
-	CORE_ADDR address;
-	char *name;
-
-	/* Ignore non-resident sections. */
-	if ((bfd_get_section_flags(bfd, sect) & (SEC_ALLOC | SEC_LOAD)) == 0)
-		return;
-
-	name = xstrdup(bfd_get_section_name(bfd, sect));
-	make_cleanup(xfree, name);
-	address = asi->base_addr + bfd_get_section_vma(bfd, sect);
-	asi->section_addrs->other[asi->sect_index].name = name;
-	asi->section_addrs->other[asi->sect_index].addr = address;
-	asi->section_addrs->other[asi->sect_index].sectindex = sect->index;
-	printf_unfiltered("\t%s_addr = %s\n", name, local_hex_string(address));
-	asi->sect_index++;
-}
-
 static void
 load_kld (char *path, CORE_ADDR base_addr, int from_tty)
 {
-	struct add_section_info asi;
+	struct section_addr_info *sap;
+	struct section_table *sections, *sections_end, *s;
 	struct cleanup *cleanup;
 	bfd *bfd;
+	int i;
 
 	/* Open the kld. */
 	bfd = bfd_openr(path, gnutarget);
@@ -244,19 +220,30 @@
 	if (bfd_get_section_by_name (bfd, ".text") == NULL)
 		error("\"%s\": can't find text section", path);
 
+	/* Build a section table from the bfd and relocate the sections. */
+	if (build_section_table (bfd, &sections, &sections_end))
+		error("\"%s\": can't find file sections", path);
+	cleanup = make_cleanup(xfree, sections);
+	for (s = sections; s < sections_end; s++) {
+		s->addr += base_addr;
+		s->endaddr += base_addr;
+	}
+
+	/* Build a section addr info to pass to symbol_file_add(). */
+	sap = build_section_addr_info_from_section_table (sections,
+	    sections_end);
+	cleanup = make_cleanup((make_cleanup_ftype *)free_section_addr_info,
+	    sap);
+
 	printf_unfiltered("add symbol table from file \"%s\" at\n", path);
-
-	/* Build a section table for symbol_file_add() from the bfd sections. */
-	asi.section_addrs = alloc_section_addr_info(bfd_count_sections(bfd));
-	cleanup = make_cleanup(xfree, asi.section_addrs);
-	asi.sect_index = 0;
-	asi.base_addr = base_addr;
-	bfd_map_over_sections(bfd, add_section, &asi);
+	for (i = 0; i < sap->num_sections; i++)
+		printf_unfiltered("\t%s_addr = %s\n", sap->other[i].name,
+		    local_hex_string(sap->other[i].addr));		
 
 	if (from_tty && (!query("%s", "")))
 		error("Not confirmed.");
 
-	symbol_file_add(path, from_tty, asi.section_addrs, 0, OBJF_USERLOADED);
+	symbol_file_add(path, from_tty, sap, 0, OBJF_USERLOADED);
 
 	do_cleanups(cleanup);
 }

-- 
John Baldwin