Re: [PATCH] Remove some obfuscation from ${arch}_skip_prologue functions

[Doug Evans <dje@google.com>]

Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > > 1) There's no need to call find_pc_partial_function before
 > > calling skip_prologue_using_sal: The first thing skip_prologue_using_sal
 > > does is call find_pc_partial_function!
 > 
 > Nowadays we have:
 > 
 >   if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
 >     {
 >       CORE_ADDR post_prologue_pc
 > 	= skip_prologue_using_sal (gdbarch, func_addr);
 > 
 >       if (post_prologue_pc != 0)
 > 	return max (pc, post_prologue_pc);
 >     }
 > 
 > so your statement is valid if PC equals to FUNC_ADDR.

There are other reasons to worry about cases when PC != FUNC_ADDR
(which is why I'm still trying to find examples of when that is true),
but I don't see how this is one of them.
Remember, the first thing skip_prologue_using_sal does is also
call find_pc_partial_function.

So what we have is:

${arch}_skip_prologue calls find_pc_partial_function (PC)
and gets back a start address, called FUNC_ADDR here.

Then,
skip_prologue_using_sal calls find_pc_partial_function (FUNC_ADDR)
and gets back a start address, called START_PC.

Then,
skip_prologue uses START_PC for the rest of the function.

So, under what circumstances is (figuratively speaking):
(find_pc_partial_function (pc)
  != find_pc_partial_function (find_pc_partial_function (pc)))
And if there is such a case, I think we've got another problem ...

btw, note that arm_skip_prologue dropped the max (pc, post_prologue_pc)
check.  It may have been accidental. I found the relevant commit and
emails, it's not clear yet why the check was dropped.

 > I don't have a
 > case that PC and FUNC_ADDR are different, but I'd like to add an assert
 > to check this, in each target's implementation of skip_prologue hook, or
 > in the callers of gdbarch_skip_prologue, something like:
 > 
 >   if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
 >     gdb_assert (pc == func_addr);

I have found two cases where, I think, ${arch}_skip_prologue can
be called with pc != func_addr: vax and ppc.
I'd be happy with simplifying the target API so that
we could have such an assert, though I'd rather not put it
in ${arch}_skip_prologue.  I currently have the problem that
the treatment of gcc vs clang is not as consistent as it could be
across all ${arch}_skip_prologue functions, so I'm on a path of keeping
as much code that should be common out of target-specific routines:
cleaning it up later is not always fun or easy.

 > Note that this assert is triggered on arm in
 > gdb.cp/re-set-overloaded.exp, that is PC is [1] but FUNC_ADDR is [2].
 > 
 > (gdb) disassemble _ZN1CC1Ei
 > Dump of assembler code for function _ZN1CC1Ev:
 >    0x0000090c <+0>:     ldr     r12, [pc, #4]   ; 0x918 <_ZN1CC1Ev+12>   <- [2]
 >    0x00000910 <+4>:     add     r12, r12, pc
 >    0x00000914 <+8>:     bx      r12
 >    0x00000918 <+12>:                    ; <UNDEFINED> instruction: 0xffffffc5
 >    0x0000091c <+0>:     ldr     r12, [pc, #4]   ; 0x928 <_ZN1CC1Ei+12>   <- [1]
 >    0x00000920 <+4>:     add     r12, r12, pc
 > 
 > AFAICS, PC is still the function address but find_pc_partial_function
 > computes the FUNC_ADDR incorrectly and it is nothing wrong about your
 > patch.

Thanks, this is good data.

I did a similar experiment on amd64-linux after writing
https://sourceware.org/ml/gdb-patches/2014-08/msg00539.html
and got no hits.

I'd be curious to see how arm_skip_prologue handles this.
What flavor of arm and what version of gcc?
I can't recreate that example with arm-linux-gnueabi-g++-4.7.
Also, can you send me gdb.log plus re-set-overloaded{,.so} ?
[don't cc the list :-)]

 > 
 > > nios2: yao@codesourcery.com
 > 
 > I tested your patch on nios2-linux, and no regression is found.
 > 
 > > tic6x:yao@codesourcery.com
 > 
 > My c6x board is dead in data center, so I can't test this patch for it.

Thanks!