Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney <cagney at gnu dot org>]

> Andrew,
>
> FYI: This is something I have started investigating but can't finish
> just yet because I have to move off to something else. You said:
>
>
> > > This kernel_trap stuff isn't applicable here.
> >
> > > > >  kernel_trap = PROC_REG_MASK (proc_desc) & 1;
> > > > >  gen_mask = kernel_trap ? 0xFFFFFFFF : PROC_REG_MASK (proc_desc);
> > > > >  float_mask = kernel_trap ? 0xFFFFFFFF : PROC_FREG_MASK (proc_desc);

Ok, first here's why this ``can't happen'':

If mips_insn32_frame_cache gets all the calls inlined we find the 
sequence (taking a birds eye view):

	-- from mips32_heuristic_proc_desc

	clear PROC_DESC
	for (pc = func_start; pc < magic; pc+=4)
	  fetch INSN at PC
	  set PROC_DESC* and TEMP_SAVED_REGS according to INSN

	-- from mips_insn32_frame_cache
	if (in prologue && !``kernel_trap'')
	  for (pc = func_start; pc < current_instruction; pc+=4)
	    fetch INSN at PC
	    set GEN_MASK according to INSN
	else
	  set GEN_MASK according to PROC_DESC

	-- from mips_insn32_frame_cache
	for (reg in gen_mask)
	  set saved regs according to GEN_MASK

what we notice (if bits are examined in detail):

- mips32_heuristic_proc_desc _never_ sets MIPS_REG_MASK&1 (i.e., 
kernel_flag) i.e., for kernel_flag to be set, a true proc_desc or

- TEMP_SAVED_REGS is never read!

- the code packs the saved register information into a scratch PROC_DESC 
only to immediatly turn around and unpack that same information (dumb!)

- when !``In any frame other than the innermost or a frame interrupted 
by a signal, we assume that all registers have been saved.  This assumes 
that all register saves in a function happen before the first function 
call.'' (i.e., when sitting in a prologue) the instructions are iterated 
over _twice_: first using the extreemly complex 
mips32_heuristic_proc_desc; and second by a remarkably simple algorithm.

- regardless of ``In any frame other than the innermost or a frame 
interrupted by a signal, we assume that all registers have been saved. 
This assumes that all register saves in a function happen before the 
first function call.'', the list of saved registers is determined by 
examining the prologue.

From other architectures we know that there's no reason to 
differentiate between the in and out-of prologue cases and all the above 
can be reduced to:

	for (pc = func_start; pc < current-pc && pc in prologue; pc+=4)
	  fetch instruction at pc
	  according to INSN set saved-regs directly

where the saved reg table is updated directly and PROC_DESC / 
kernel_flag are both eliminated.

--

Now why the theory appears broken:

> But in fact for a reason I don't completely understand just yet, we
> do have kernel_trap set occasionally (I measured ~300+ times in our
> testsuite). For instance, in all-bin.exp, debugging all-types:
>
>         (gdb) break main
>         (gdb) run 
>         Starting program: /[...]/all-types 
>         
>         Breakpoint 1, main () at all-types.c:35
>         35          dummy();
>         (gdb) next
>  !! ->  *** DEBUG: kernel_trap in mips_insn32_frame_cache ***
>         36          return 0;
>
> I don't have all the details just yet, but my semi-educated guess
> is that the sequence of events that lead to this is the following:
>    . User enters next command
>    . GDB does a next, and waits for events
>    . We land inside dummy (unverified)

That would trigger a frame unwind (to get the caller PC so that the 
return-address breakpoint can be set) which most likely goes through 
this path:

> static const struct frame_unwind *
> mips_mdebug_frame_sniffer (struct frame_info *next_frame)
> ...
>   /* In any frame other than the innermost or a frame interrupted by a
>      signal, we assume that all registers have been saved.  This
>      assumes that all register saves in a function happen before the
>      first function call.  */
>   if (!in_prologue (pc, PROC_LOW_ADDR (proc_desc)))
>     return &mips_mdebug_frame_unwind;
>
>   return NULL;

and lead to mips_insn32* being chosen.  mips_insn32 would then end up 
going through this path:
> static mips_extra_func_info_t
> find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame, int cur_frame)
> {
>   mips_extra_func_info_t proc_desc;
>   CORE_ADDR startaddr = 0;
>
>   proc_desc = non_heuristic_proc_desc (pc, &startaddr);
>
>   if (proc_desc)
>     {
>       /* IF this is the topmost frame AND
>        * (this proc does not have debugging information OR
>        * the PC is in the procedure prologue)
>        * THEN create a "heuristic" proc_desc (by analyzing
>        * the actual code) to replace the "official" proc_desc.
>        */
>       if (next_frame == NULL)
> 	{
> 	  struct symtab_and_line val;
> 	  struct symbol *proc_symbol =
> 	    PROC_DESC_IS_DUMMY (proc_desc) ? 0 : PROC_SYMBOL (proc_desc);
>
> 	  if (proc_symbol)
> 	    {
> 	      val = find_pc_line (BLOCK_START
> 				  (SYMBOL_BLOCK_VALUE (proc_symbol)), 0);
> 	      val.pc = val.end ? val.end : pc;
> 	    }
> 	  if (!proc_symbol || pc < val.pc)
> 	    {
> 	      mips_extra_func_info_t found_heuristic =
> 		heuristic_proc_desc (PROC_LOW_ADDR (proc_desc),
> 				     pc, next_frame, cur_frame);
> 	      if (found_heuristic)
> 		proc_desc = found_heuristic;
> 	    }
> 	}
>     }
>   else

Remember: there is an mdebug PROC_DESC but it was being ignored; 
next_frame is never NULL.  That leads to the mdebug PROC_DESC being 
used.  Outch!

What I don't get is how come this bit of:

>   /* Not sure exactly what kernel_trap means, but if it means the
>      kernel saves the registers without a prologue doing it, we better
>      not examine the prologue to see whether registers have been saved
>      yet.  */
>   kernel_trap = PROC_REG_MASK (proc_desc) & 1;
>   if (kernel_trap)
>     return &mips_mdebug_frame_unwind;

didn't kick in forcing the use of the mdebug unwinder.

Anyway, eew!

This is where we need to be ruthless with the code - just inline the lot 
because (even when the testresults regress) we can see that it is more 
correct and more maintainable then what we're currently looking at.  We 
need to push the mips-tdep.c file out of the local minima that it has 
become trapped in.

Andrew