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

Doug Evans
2014-09-04T17:42:45Z

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!