Re: [PATCH v2 1/2] ISA bit treatment on the MIPS platform

["Maciej W. Rozycki" <macro at codesourcery dot com>]

Joel, Doug --

On Sun, 16 Nov 2014, Joel Brobecker wrote:

> > Index: gdb-fsf-trunk-quilt/gdb/gdbarch.sh
> > ===================================================================
> > --- gdb-fsf-trunk-quilt.orig/gdb/gdbarch.sh	2014-10-03 13:52:46.000000000 +0100
> > +++ gdb-fsf-trunk-quilt/gdb/gdbarch.sh	2014-10-03 14:50:26.000000000 +0100
> > @@ -635,8 +635,11 @@ m:int:in_solib_return_trampoline:CORE_AD
> >  # which don't suffer from that problem could just let this functionality
> >  # untouched.
> >  m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0
> > -f:void:elf_make_msymbol_special:asymbol *sym, struct minimal_symbol *msym:sym, msym::default_elf_make_msymbol_special::0
> > +F:void:elf_make_msymbol_special:asymbol *sym, struct minimal_symbol *msym:sym, msym
> >  f:void:coff_make_msymbol_special:int val, struct minimal_symbol *msym:val, msym::default_coff_make_msymbol_special::0
> > +f:void:make_symbol_special:struct symbol *sym, struct objfile *objfile:sym, objfile::default_make_symbol_special::0
> > +f:CORE_ADDR:adjust_dwarf2_addr:CORE_ADDR pc:pc::default_adjust_dwarf2_addr::0
> > +f:CORE_ADDR:adjust_dwarf2_line:CORE_ADDR addr, int rel:addr, rel::default_adjust_dwarf2_line::0
> 
> I know we haven't been all that good in the past at documenting
> gdbarch methods, but new entries should be fully documented. That way,
> all arch-specific implementations can then document with a reference
> to the documentation in gdbarch.sh/gdbarch.h. Would you mind documenting
> the ones you're adding?
> 
> This is not a requirement for your patch, but if you happen to be able
> to quickly document elf_make_msymbol_special as well, that would be
> a very welcome and appreciated change.

 I have now added these descriptions, with some bias towards the MIPS 
specifics as they are what I am most familiar with.  Please let me know 
if you think they might be further improved.

> > +/* Recalculate the line record requested so that the resulting PC has the
> > +   ISA bit set correctly, used by DWARF-2 machinery.  */
> > +
> > +static CORE_ADDR
> > +mips_adjust_dwarf2_line (CORE_ADDR addr, int rel)
> > +{
> > +  static CORE_ADDR adj_pc;
> > +  static CORE_ADDR pc;
> > +  CORE_ADDR isa_pc;
> > +
> > +  pc = rel ? pc + addr : addr;
> > +  isa_pc = mips_adjust_dwarf2_addr (pc);
> > +  addr = rel ? isa_pc - adj_pc : isa_pc;
> > +  adj_pc = isa_pc;
> > +  return addr;
> > +}
> 
> I got to stare that his code for quite a while, because of the use
> of the private static variables, which means that the result of
> a call to that function depends on the previous call. I am guessing
> that this is the reason behind the following hunk earlier in the patch:
> 
>     -      CORE_ADDR address = 0;
>     +      CORE_ADDR address = gdbarch_adjust_dwarf2_line (gdbarch, 0, 0);
> 
> (to initialize the context).

 Correct, this simulates an implicit line entry at the start of 
processing so that a possible initial relative address adjustment line 
information entry works correctly.

> I think we all know that if makes the implementation a little harder
> to maintain in the long run, but after thinking over it for a while,
> I tend to like the fact that this helps making the chhanges to
> dwarf2read.c a little simpler, with negative impact being only on
> the MIPS target. So while I wish things were perfect, I'm have this
> feeling that the above is a "good deal" for core GDB ;-).

 Yes, as I already noted in my other reply today made to a similar 
concern Doug made, I concluded making it as simple as possible and 
putting the most burden on the MIPS backend will incur the least amount 
of long maintenance effort.  Indeed, having persistent state kept in 
local static variables is a bit obscure, but I think this is a "good 
deal", as you say.

> > +++ gdb-fsf-trunk-quilt/gdb/solib.c	2014-10-03 14:50:26.398945943 +0100
> > @@ -1444,8 +1444,28 @@ gdb_bfd_lookup_symbol_from_symtab (bfd *
> >  
> >  	  if (match_sym (sym, data))
> >  	    {
> > +	      struct gdbarch *gdbarch = target_gdbarch ();
> > +	      symaddr = sym->value;
> > +
> > +	      /* 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
> > +		  && gdbarch_elf_make_msymbol_special_p (gdbarch))
> > +		{
> > +		  struct minimal_symbol msym;
> > +
> > +		  memset (&msym, 0, sizeof (msym));
> > +		  SET_MSYMBOL_VALUE_ADDRESS (&msym, symaddr);
> > +		  gdbarch_elf_make_msymbol_special (gdbarch, sym, &msym);
> > +		  symaddr = MSYMBOL_VALUE_RAW_ADDRESS (&msym);
> > +		}
> > +
> >  	      /* BFD symbols are section relative.  */
> > -	      symaddr = sym->value + sym->section->vma;
> > +	      symaddr += sym->section->vma;
> >  	      break;
> 
> FTR: I think I had some reservations about this part of the code,
> and you improved it to only do the work if
> gdbarch_elf_make_msymbol_special_p, which is a nice improvement
> anyway. But looking at it more closely, I probably missed at the time
> the fact that the minimal_symbol has a local scope. (duh!)

 I thought you actually were (quite validly IMO) concerned about all the 
preparatory steps made across all targets to set up `msym', just to do 
nothing about it then.

 So here are the changes I made to address the concerns you both 
expressed, as an incremental patch for easier review.  If this looks 
good to you, then I'll fold it into the original change and commit (the 
comment addition for `elf_make_msymbol_special' may better go in as a 
separate change though).

 Thanks for your input.

  Maciej

gdb-mips16-isa-bit-update.diff
Index: gdb-fsf-trunk-quilt/gdb/dwarf2read.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/dwarf2read.c	2014-12-04 13:55:33.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/dwarf2read.c	2014-12-04 18:22:26.428924108 +0000
@@ -17323,7 +17323,11 @@ dwarf_decode_lines_1 (struct line_header
   /* Read the statement sequences until there's nothing left.  */
   while (line_ptr < line_end)
     {
-      /* state machine registers  */
+      /* State machine registers.  Call `gdbarch_adjust_dwarf2_line'
+         on the initial 0 address as if there was a line entry for it
+         so that the backend has a chance to adjust it and also record
+         it in case it needs it.  This is currently used by MIPS code,
+         cf. `mips_adjust_dwarf2_line'.  */
       CORE_ADDR address = gdbarch_adjust_dwarf2_line (gdbarch, 0, 0);
       unsigned int file = 1;
       unsigned int line = 1;
Index: gdb-fsf-trunk-quilt/gdb/gdbarch.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbarch.h	2014-12-04 13:55:33.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/gdbarch.h	2014-12-04 21:34:44.678927696 +0000
@@ -690,6 +690,14 @@ typedef int (gdbarch_in_function_epilogu
 extern int gdbarch_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR addr);
 extern void set_gdbarch_in_function_epilogue_p (struct gdbarch *gdbarch, gdbarch_in_function_epilogue_p_ftype *in_function_epilogue_p);
 
+/* Process an ELF symbol in the minimal symbol table in a backend-specific
+   way.  Normally this hook is supposed to do nothing, however if required,
+   then this hook can be used to apply tranformations to symbols that are
+   considered special in some way.  For example the MIPS backend uses it
+   to interpret `st_other' information to mark compressed code symbols so
+   that they can be treated in the appropriate manner in the processing of
+   the main symbol table and DWARF-2 records. */
+
 extern int gdbarch_elf_make_msymbol_special_p (struct gdbarch *gdbarch);
 
 typedef void (gdbarch_elf_make_msymbol_special_ftype) (asymbol *sym, struct minimal_symbol *msym);
@@ -700,14 +708,41 @@ typedef void (gdbarch_coff_make_msymbol_
 extern void gdbarch_coff_make_msymbol_special (struct gdbarch *gdbarch, int val, struct minimal_symbol *msym);
 extern void set_gdbarch_coff_make_msymbol_special (struct gdbarch *gdbarch, gdbarch_coff_make_msymbol_special_ftype *coff_make_msymbol_special);
 
+/* Process a symbol in the main symbol table in a backend-specific way.
+   Normally this hook is supposed to do nothing, however if required,
+   then this hook can be used to apply tranformations to symbols that
+   are considered special in some way.  This is currently used by the
+   MIPS backend to make sure compressed code symbols have the ISA bit
+   set.  This in turn is needed for symbol values seen in GDB to match
+   the values used at the runtime by the program itself, for function
+   and label references. */
+
 typedef void (gdbarch_make_symbol_special_ftype) (struct symbol *sym, struct objfile *objfile);
 extern void gdbarch_make_symbol_special (struct gdbarch *gdbarch, struct symbol *sym, struct objfile *objfile);
 extern void set_gdbarch_make_symbol_special (struct gdbarch *gdbarch, gdbarch_make_symbol_special_ftype *make_symbol_special);
 
+/* Adjust the address retrieved from a DWARF-2 record other than a line
+   entry in a backend-specific way.  Normally this hook is supposed to
+   return the address passed unchanged, however if that is incorrect for
+   any reason, then this hook can be used to fix the address up in the
+   required manner.  This is currently used by the MIPS backend to make
+   sure addresses in FDE, range records, etc. referring to compressed
+   code have the ISA bit set, matching line information and the symbol
+   table. */
+
 typedef CORE_ADDR (gdbarch_adjust_dwarf2_addr_ftype) (CORE_ADDR pc);
 extern CORE_ADDR gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, CORE_ADDR pc);
 extern void set_gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, gdbarch_adjust_dwarf2_addr_ftype *adjust_dwarf2_addr);
 
+/* Adjust the address updated by a line entry in a backend-specific way.
+   Normally this hook is supposed to return the address passed unchanged,
+   however in the case of inconsistencies in these records, this hook can
+   be used to fix them up in the required manner.  This is currently used
+   by the MIPS backend to make sure all line addresses in compressed code
+   are presented with the ISA bit set, which is not always the case.  This
+   in turn ensures breakpoint addresses are correctly matched against the
+   stop PC. */
+
 typedef CORE_ADDR (gdbarch_adjust_dwarf2_line_ftype) (CORE_ADDR addr, int rel);
 extern CORE_ADDR gdbarch_adjust_dwarf2_line (struct gdbarch *gdbarch, CORE_ADDR addr, int rel);
 extern void set_gdbarch_adjust_dwarf2_line (struct gdbarch *gdbarch, gdbarch_adjust_dwarf2_line_ftype *adjust_dwarf2_line);
Index: gdb-fsf-trunk-quilt/gdb/gdbarch.sh
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbarch.sh	2014-12-04 13:55:33.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/gdbarch.sh	2014-12-04 21:34:21.168924205 +0000
@@ -635,10 +635,41 @@ m:int:in_solib_return_trampoline:CORE_AD
 # which don't suffer from that problem could just let this functionality
 # untouched.
 m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0
+# Process an ELF symbol in the minimal symbol table in a backend-specific
+# way.  Normally this hook is supposed to do nothing, however if required,
+# then this hook can be used to apply tranformations to symbols that are
+# considered special in some way.  For example the MIPS backend uses it
+# to interpret \`st_other' information to mark compressed code symbols so
+# that they can be treated in the appropriate manner in the processing of
+# the main symbol table and DWARF-2 records.
 F:void:elf_make_msymbol_special:asymbol *sym, struct minimal_symbol *msym:sym, msym
 f:void:coff_make_msymbol_special:int val, struct minimal_symbol *msym:val, msym::default_coff_make_msymbol_special::0
+# Process a symbol in the main symbol table in a backend-specific way.
+# Normally this hook is supposed to do nothing, however if required,
+# then this hook can be used to apply tranformations to symbols that
+# are considered special in some way.  This is currently used by the
+# MIPS backend to make sure compressed code symbols have the ISA bit
+# set.  This in turn is needed for symbol values seen in GDB to match
+# the values used at the runtime by the program itself, for function
+# and label references.
 f:void:make_symbol_special:struct symbol *sym, struct objfile *objfile:sym, objfile::default_make_symbol_special::0
+# Adjust the address retrieved from a DWARF-2 record other than a line
+# entry in a backend-specific way.  Normally this hook is supposed to
+# return the address passed unchanged, however if that is incorrect for
+# any reason, then this hook can be used to fix the address up in the
+# required manner.  This is currently used by the MIPS backend to make
+# sure addresses in FDE, range records, etc. referring to compressed
+# code have the ISA bit set, matching line information and the symbol
+# table.
 f:CORE_ADDR:adjust_dwarf2_addr:CORE_ADDR pc:pc::default_adjust_dwarf2_addr::0
+# Adjust the address updated by a line entry in a backend-specific way.
+# Normally this hook is supposed to return the address passed unchanged,
+# however in the case of inconsistencies in these records, this hook can
+# be used to fix them up in the required manner.  This is currently used
+# by the MIPS backend to make sure all line addresses in compressed code
+# are presented with the ISA bit set, which is not always the case.  This
+# in turn ensures breakpoint addresses are correctly matched against the
+# stop PC.
 f:CORE_ADDR:adjust_dwarf2_line:CORE_ADDR addr, int rel:addr, rel::default_adjust_dwarf2_line::0
 v:int:cannot_step_breakpoint:::0:0::0
 v:int:have_nonsteppable_watchpoint:::0:0::0
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2014-12-04 13:55:33.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2014-12-04 22:25:22.498924888 +0000
@@ -414,23 +414,29 @@ msymbol_is_micromips (struct minimal_sym
 }
 
 /* Set the ISA bit in the main symbol too, complementing the corresponding
-   minimal symbol setting and reflecting the run-time value of the symbol.  */
+   minimal symbol setting and reflecting the run-time value of the symbol.
+   The need for comes from the ISA bit having been cleared as code in
+   `_bfd_mips_elf_symbol_processing' separated it into the ELF symbol's
+   `st_other' STO_MIPS16 or STO_MICROMIPS annotation, making the values
+   of symbols referring to compressed code different in GDB to the values
+   used by actual code.  That in turn makes them evaluate incorrectly in
+   expressions, producing results different to what the same expressions
+   yield when compiled into the program being debugged.  */
 
 static void
 mips_make_symbol_special (struct symbol *sym, struct objfile *objfile)
 {
   if (SYMBOL_CLASS (sym) == LOC_BLOCK)
     {
+      /* We are in symbol reading so it is OK to cast away constness.  */
+      struct block *block = (struct block *) SYMBOL_BLOCK_VALUE (sym);
       CORE_ADDR compact_block_start;
       struct bound_minimal_symbol msym;
 
-      compact_block_start = BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) | 1;
+      compact_block_start = BLOCK_START (block) | 1;
       msym = lookup_minimal_symbol_by_pc (compact_block_start);
       if (msym.minsym && !msymbol_is_mips (msym.minsym))
 	{
-	  /* We are in symbol reading so it is OK to cast away constness.  */
-	  struct block *block = (struct block *) SYMBOL_BLOCK_VALUE (sym);
-
 	  BLOCK_START (block) = compact_block_start;
 	}
     }
@@ -1248,7 +1254,11 @@ mips_pc_isa (struct gdbarch *gdbarch, CO
     }
 }
 
-/* Set the ISA bit correctly in the PC, used by DWARF-2 machinery.  */
+/* Set the ISA bit correctly in the PC, used by DWARF-2 machinery.
+   The need for comes from the ISA bit having been cleared, making
+   addresses in FDE, range records, etc. referring to compressed code
+   different to those in line information, the symbol table and finally
+   the PC register.  That in turn confuses many operations.  */
 
 static CORE_ADDR
 mips_adjust_dwarf2_addr (CORE_ADDR pc)
@@ -1257,8 +1267,39 @@ mips_adjust_dwarf2_addr (CORE_ADDR pc)
   return mips_pc_is_mips (pc) ? pc : make_compact_addr (pc);
 }
 
-/* Recalculate the line record requested so that the resulting PC has the
-   ISA bit set correctly, used by DWARF-2 machinery.  */
+/* Recalculate the line record requested so that the resulting PC has
+   the ISA bit set correctly, used by DWARF-2 machinery.  The need for
+   this adjustment comes from some records associated with compressed
+   code having the ISA bit cleared, most notably at function prologue
+   ends.  The ISA bit is in this context retrieved from the minimal
+   symbol covering the address requested, which in turn has been
+   constructed from the binary's symbol table rather than DWARF-2
+   information.  The correct setting of the ISA bit is required for
+   breakpoint addresses to correctly match against the stop PC.
+
+   As line entries can specify relative address adjustments we need to
+   keep track of the absolute value of the last line address recorded
+   in line information, so that we can calculate the actual address to
+   apply the ISA bit adjustment to.  We use PC for this tracking and
+   keep the original address there.
+
+   As such relative address adjustments can be odd within compressed
+   code we need to keep track of the last line address with the ISA
+   bit adjustment applied too, as the original address may or may not
+   have had the ISA bit set.  We use ADJ_PC for this tracking and keep
+   the adjusted address there.
+
+   For relative address adjustments we then use these variables to
+   calculate the address intended by line information, which will be
+   PC-relative, and return an updated adjustment carrying ISA bit
+   information, which will be ADJ_PC-relative.  For absolute address
+   adjustments we just return the same address that we store in ADJ_PC
+   too.
+
+   As the first line entry can be relative to an implied address value
+   of 0 we need to have the initial address set up that we store in PC
+   and ADJ_PC.  This is arranged with a call from `dwarf_decode_lines_1'
+   that sets PC to 0 and ADJ_PC accordingly, usually 0 as well.  */
 
 static CORE_ADDR
 mips_adjust_dwarf2_line (CORE_ADDR addr, int rel)