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

Maciej W. Rozycki
2014-12-04T15:31:12Z

On Mon, 17 Nov 2014, Doug Evans wrote:

> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
> > [...]
> > 2014-10-06 Maciej W. Rozycki <macro@codesourcery.com>
> > Maciej W. Rozycki <macro@mips.com>
> > Pedro Alves <pedro@codesourcery.com>
> >
> > * gdbarch.sh (elf_make_msymbol_special): Change type to `F',
> > remove `predefault' and `invalid_p' initializers.
> > (make_symbol_special): New architecture method.
> > (adjust_dwarf2_addr, adjust_dwarf2_line): Likewise.
> > (objfile, symbol): New declarations.
> > * arch-utils.h (default_elf_make_msymbol_special): Remove
> > prototype.
> > (default_make_symbol_special): New prototype.
> > (default_adjust_dwarf2_addr): Likewise.
> > (default_adjust_dwarf2_line): Likewise.
> > * mips-tdep.h (mips_unmake_compact_addr): New prototype.
> > * arch-utils.c (default_elf_make_msymbol_special): Remove
> > function.
> > (default_make_symbol_special): New function.
> > (default_adjust_dwarf2_addr): Likewise.
> > (default_adjust_dwarf2_line): Likewise.
> > * dwarf2-frame.c (decode_frame_entry_1): Call
> > `gdbarch_adjust_dwarf2_addr'.
> > * dwarf2loc.c (dwarf2_find_location_expression): Likewise.
> > * dwarf2read.c (create_addrmap_from_index): Likewise.
> > (process_psymtab_comp_unit_reader): Likewise.
> > (add_partial_symbol): Likewise.
> > (add_partial_subprogram): Likewise.
> > (process_full_comp_unit): Likewise.
> > (read_file_scope): Likewise.
> > (read_func_scope): Likewise. Call `gdbarch_make_symbol_special'.
> > (read_lexical_block_scope): Call `gdbarch_adjust_dwarf2_addr'.
> > (read_call_site_scope): Likewise.
> > (dwarf2_ranges_read): Likewise.
> > (dwarf2_record_block_ranges): Likewise.
> > (read_attribute_value): Likewise.
> > (dwarf_decode_lines_1): Call `gdbarch_adjust_dwarf2_line'.
> > (new_symbol_full): Call `gdbarch_adjust_dwarf2_addr'.
> > * elfread.c (elf_symtab_read): Don't call
> > `gdbarch_elf_make_msymbol_special' if unset.
> > * mips-linux-tdep.c (micromips_linux_sigframe_validate): Strip
> > the ISA bit from the PC.
> > * mips-tdep.c (mips_unmake_compact_addr): New function.
> > (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.
> >
> > 2014-10-06 Maciej W. Rozycki <macro@codesourcery.com>
> >
> > gdb/testsuite/
> > * gdb.base/func-ptrs.c: New file.
> > * gdb.base/func-ptrs.exp: New file.
>
> Ummm, bleah.

Yeah, that's a consequence of a bad decision made (or the lack of one
and the resulting semantics that spontaneously happened) many years ago
as MIPS16 support was implemented. References to compressed code are
made in an inconsistent way across DWARF records with information lost
from some of them. That information has to be recovered from the symbol
table.

The missing information could be stored separately in DWARF records I
suppose, but that won't help with many binaries out there; MIPS16
support has been there for well over a decade -- our first record is
from Jan 1997 and support in GCC/GAS must have been there since around
that time too.

> Going forward dwarf2*.c are going to be measurably more of a pain
> to maintain than they already are.
> I notice a fair bit of mips code gets a lot simpler.

And some stuff actually starts working, that never did before. I
posted the full story along the original patch submission:

http://sourceware.org/ml/gdb-patches/2012-05/msg00515.html

> Such is life I guess.

I tried hard figuring out a way that would avoid making changes to
generic DWARF code and could not find one. Once the DWARF records have
been processed there doesn't appear to me to be a way for places across
GDB's generic code to gather the pieces of information required without
turning upside down those places for a change.

> Just a few comments.
>
> >[...]
> > 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 don't know where the best place to put this is, but the adjust_dwarf2
> functions need a lot more documentation. Why do they exist?
> What problem do they solve? Something along those lines.
> References to existing documentation is fine, as long as they're sufficient.

The source code has so far been the documentation I am afraid. Which I
think also contributes to the mess. Implied bits are buried in GCC in
gcc/config/mips/mips.h and libgcc/unwind-dw2.c for example, see the
comment around `MASK_RETURN_ADDR' in the former and the comment at the
beginning of `execute_cfa_program' in the latter.

The assumptions made in these both places make issues nasty, adding 1
to the return address in the latter place is incompatible with how the
ISA bit is interpreted everywhere else. This function post-dates the
introduction of the MIPS16 instruction set I believe and therefore I
think it should have been designed with the issues around it in mind,
e.g. I used the patch below in the early stage of investigation of
possible solutions for the problem addressed here.

> I realize I can find that out with some manual labor, e.g., finding
> which arches provide an implementation of these functions, and reading them,
> but even when I get to mips-tdep.c I'm left wondering *why*
> the mips port needs this (e.g., why can't the problem be solved
> differently?). I'm more familiar with arm/thumb, and of course
> the first thing that comes to mind is that arm/thumb doesn't need this
> (maybe it does, but we've gotten this far without it).

I suspect that it does, but it can be found out easily as this is why I
included gdb.base/func-ptrs.exp with the change. Any target that fails
the test cases it contains will need addressing the problem, possibly
just by implementing the equivalent of the MIPS bits posted here.

> It would help
> to educate the reader of this code as to why mips is different.
> Maybe this patch is the best solution. If so, and I'm going to assume that
> it is, then let's get this documented in the code.
> Even just a reference to existing docs is fine.
> mips-tdep.c is a big file though - I don't want to have to read all
> of it just to understand why adjust_dwarf2_line exists.

I'll try summarising the key points for the new hooks in gdbarch.sh
(that is propagated by the script to gdbarch.h) and for the MIPS
implementation in mips-tdep.c. I'll post the final version for you (and
Joel) to comment on before I push it, in the next few days.

> > Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
> > ===================================================================
> > --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2014-10-03 13:52:46.000000000 +0100
> > +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2014-10-06 00:11:55.028611249 +0100
> >[...]
> > @@ -391,6 +406,29 @@ msymbol_is_micromips (struct minimal_sym
> > return MSYMBOL_TARGET_FLAG_2 (msym);
> > }
> >
> > +/* Set the ISA bit in the main symbol too, complementing the corresponding
> > + minimal symbol setting and reflecting the run-time value of the symbol. */
> > +
> > +static void
> > +mips_make_symbol_special (struct symbol *sym, struct objfile *objfile)
> > +{
> > + if (SYMBOL_CLASS (sym) == LOC_BLOCK)
> > + {
> > + CORE_ADDR compact_block_start;
> > + struct bound_minimal_symbol msym;
> > +
> > + compact_block_start = BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) | 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;
> > + }
> > + }
> > +}
>
> This function would be easier to read if the assignment to block
> was moved up and its value used in both invocations of BLOCK_START.

Fixed. It looks to me like an outcome from the long history of this
function (dating back to Jan 2008) and the number of adjustments it
gathered as the structures it uses evolved. In the process I missed the
possibility for this simplification.

> >[...]
> > +/* 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;
> > +}
>
> If this function was a "method", we could remove the "static" vars,
> but this is ok I guess.
> There's magic going on here, carrying the value of pc,adj_pc
> from one call to the next. There's a rule here that this will
> be called with rel == 0, before any calls with rel != 0.
> It's kinda obvious now, but it would have helped this reader to see
> it written down.

Indeed, already noticed by Joel. This piece gave me a bit of a
headache and after considering available alternatives I decided to put
the least burden on our DWARF machinery and contain all the stuff within
the MIPS backend. This way people outside the MIPS world do not have to
be bothered with it, and I think code is also relatively simple, which
is going to help with long-term maintenance.

The only drawback is backend's state has to be initialised and
consequently an initial call with `rel == 0' is required. I'll document
it at the call site and here as well.

And here's the experimental patch for GCC EH unwinder I referred to
above, published for posterity.

Maciej

gcc-mips-unwind-keep-isa-bit.diff
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.h 2012-05-01 20:44:04.000000000 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.h 2012-05-02 23:44:40.375576966 +0100
@@ -2049,6 +2049,7 @@ enum reg_class

#define RETURN_ADDR_RTX mips_return_addr

+#if 0
/* Mask off the MIPS16 ISA bit in unwind addresses.

The reason for this is a little subtle. When unwinding a call,
@@ -2076,6 +2077,9 @@ enum reg_class
Masking off the ISA bit means that the target-independent code
will search for "(RA & -2) - 1", which is guaranteed to be odd. */
#define MASK_RETURN_ADDR GEN_INT (-2)
+#else
+#define MIN_INSN_SIZE 2
+#endif

/* Similarly, don't use the least-significant bit to tell pointers to
Index: gcc-fsf-trunk-quilt/libgcc/unwind-dw2.c
===================================================================
--- gcc-fsf-trunk-quilt.orig/libgcc/unwind-dw2.c 2012-05-01 20:41:24.275429140 +0100
+++ gcc-fsf-trunk-quilt/libgcc/unwind-dw2.c 2012-05-02 23:44:40.385574339 +0100
@@ -927,7 +927,8 @@ execute_cfa_program (const unsigned char
In signal frames, return address is after last completed instruction,
so we add 1 to return address to make the comparison <=. */
while (insn_ptr < insn_end
- && fs->pc < context->ra + _Unwind_IsSignalFrame (context))
+ && fs->pc <= context->ra - (_Unwind_IsSignalFrame (context)
+ ? 0 : MIN_INSN_SIZE))
{
unsigned char insn = *insn_ptr++;
_uleb128_t reg, utmp;
@@ -1177,7 +1178,8 @@ uw_frame_state_for (struct _Unwind_Conte
if (context->ra == 0)
return _URC_END_OF_STACK;

- fde = _Unwind_Find_FDE (context->ra + _Unwind_IsSignalFrame (context) - 1,
+ fde = _Unwind_Find_FDE (context->ra - (_Unwind_IsSignalFrame (context)
+ ? 0 : MIN_INSN_SIZE),
&context->bases);
if (fde == NULL)
{