> I propose therefore to accept the existing inconsistencies and deal > with them entirely within GDB. I have figured out that the ISA bit > lost in various places can still be recovered as long as we have > symbol information -- that'll have the st_other attribute correctly > set to one of standard MIPS/MIPS16/microMIPS.
That seems reasonable to me. > 2012-05-14 Maciej W. Rozycki <ma...@codesourcery.com> > Maciej W. Rozycki <ma...@mips.com> > Pedro Alves <pe...@codesourcery.com> > > * gdbarch.sh (make_symbol_special): New architecture method. > (adjust_dwarf2_addr, adjust_dwarf2_line): Likewise. > (objfile, symbol): New declarations. > * arch-utils.h (default_make_symbol_special): New prototype. > (default_adjust_dwarf2_addr): Likewise. > (default_adjust_dwarf2_line): Likewise. > * arch-utils.c (default_make_symbol_special): New function. > * dwarf2-frame.c (decode_frame_entry_1): Call > gdbarch_adjust_dwarf2_addr. > * dwarf2loc.c (dwarf2_find_location_expression): Likewise. > * dwarf2read.c (read_func_scope): Call > gdbarch_make_symbol_special. > (dwarf2_ranges_read): Call gdbarch_adjust_dwarf2_addr. > (read_attribute_value): Likewise. > (dwarf_decode_lines_1): Call gdbarch_adjust_dwarf2_line. > * mips-tdep.c (mips_elf_make_msymbol_special): Set the ISA bit > in the symbol's address appropriately. > (mips_make_symbol_special): New function. > (mips_pc_is_mips): Set the ISA bit before symbol lookup. > (mips_pc_is_mips16): Likewise. > (mips_pc_is_micromips): Likewise. > (mips_pc_isa): Likewise. > (mips_adjust_dwarf2_addr): New function. > (mips_adjust_dwarf2_line): Likewise. > (mips_read_pc, mips_unwind_pc): Keep the ISA bit. > (mips_addr_bits_remove): Likewise. > (mips_skip_trampoline_code): Likewise. > (mips_write_pc): Don't set the ISA bit. > (mips_eabi_push_dummy_call): Likewise. > (mips_o64_push_dummy_call): Likewise. > (mips_gdbarch_init): Install mips_make_symbol_special, > mips_adjust_dwarf2_addr and mips_adjust_dwarf2_line gdbarch > handlers. > * solib.c (gdb_bfd_lookup_symbol_from_symtab): Get > target-specific symbol address adjustments. > * gdbarch.h: Regenerate. > * gdbarch.c: Regenerate. >From the ChangeLog entry, it seems like Pedro was involved in the making of that patch, so perhaps he could be a good reviewer? There are also changes in dwarf2*, so perhaps people such as Doug and Tom could also be involved.... Just some minor comments below: > gdb-mips16-isa-bit.diff > Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c > =================================================================== > --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2012-05-14 16:00:33.000000000 > +0100 > +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2012-05-14 16:02:02.235560558 > +0100 > @@ -358,9 +358,15 @@ mips_elf_make_msymbol_special (asymbol * > return; > > if (ELF_ST_IS_MICROMIPS (elfsym->internal_elf_sym.st_other)) > - MSYMBOL_TARGET_FLAG_2 (msym) = 1; > + { > + MSYMBOL_TARGET_FLAG_2 (msym) = 1; > + SYMBOL_VALUE_ADDRESS (msym) |= 1; > + } > else if (ELF_ST_IS_MIPS16 (elfsym->internal_elf_sym.st_other)) > - MSYMBOL_TARGET_FLAG_1 (msym) = 1; > + { > + MSYMBOL_TARGET_FLAG_1 (msym) = 1; > + SYMBOL_VALUE_ADDRESS (msym) |= 1; > + } This remark is not to be considered as part of this patch's review, since this is already an established practice, but I think we could do better than using magic numbers for those flags. I understand they have to be that way in the common code, but perhaps mips-tdep could define aliases? Something like MSYMBOL_MIPS_TARGET_FLAG_BLAH? > Index: gdb-fsf-trunk-quilt/gdb/arch-utils.c [...] > +void > +default_make_symbol_special (struct symbol *sym, struct objfile *objfile) Can you put a comment at the start of all these new functions to say that the documentation is in the header file? For instance, I think we typically say: /* See arch-utils.h. */ This is to help someone reviewing the code to know that there is in fact documentation. > +/* Do nothing version of make_symbol_special. */ > + > +void default_make_symbol_special (struct symbol *sym, struct objfile > *objfile); > + > +/* Do nothing version of adjust_dwarf2_addr. */ > + > +CORE_ADDR default_adjust_dwarf2_addr (CORE_ADDR pc); > + > +/* Do nothing version of adjust_dwarf2_line. */ > + > +CORE_ADDR default_adjust_dwarf2_line (CORE_ADDR addr, int rel); I would definitely mention gdbarch in the function comment. Prior art uses: /* Default implementation of gdbarch_displaced_hw_singlestep. */ Should we remain consistent with that? It can be a mix of both, saying what method it implements, and then what it actually does, Eg: /* Default version of gdbarch_make_symbol_special (does nothing). */ > Index: gdb-fsf-trunk-quilt/gdb/solib.c > =================================================================== > --- gdb-fsf-trunk-quilt.orig/gdb/solib.c 2012-05-14 15:56:45.000000000 > +0100 > +++ gdb-fsf-trunk-quilt/gdb/solib.c 2012-05-14 16:01:34.645558637 +0100 > @@ -1384,8 +1384,27 @@ gdb_bfd_lookup_symbol_from_symtab (bfd * > > if (match_sym (sym, data)) > { > + symaddr = sym->value; > + > + /* macro/2012-04-20: Some ELF targets fiddle with addresses > + of symbols they consider special. They use minimal symbols > + to do that and this is needed for correct breakpoint > + placement, but we do not have full data here to build a > + complete minimal symbol, so just set the address and let the > + targets cope with that. */ > + if (bfd_get_flavour (abfd) == bfd_target_elf_flavour) > + { > + struct minimal_symbol msym; > + > + memset (&msym, 0, sizeof (msym)); > + SYMBOL_VALUE_ADDRESS (&msym) = symaddr; > + gdbarch_elf_make_msymbol_special (target_gdbarch, > + sym, &msym); > + symaddr = SYMBOL_VALUE_ADDRESS (&msym); > + } Is there a way we could avoid that block if gdbarch_elf_make_msymbol_special points to the default implementation? It just seems sad to be doing all this work of creating a temporary symbol for every solib symbol on non-mips targets... One possible way would have been to provide no default value for gdbarch_elf_make_msymbol_special, but then it makes the calling of that method a little trickier. That could be worked around by changing the default value of gdbarch_elf_make_msymbol_special to NULL, and then having an extra wrapper function that call the gdbarch method if not NULL or else does nothing. -- Joel