[RFA] Add missing source location info to thunks

GCC generates non-virtual thunks directly to assembly code, which
causes a couple of problems. First, it doesn't add source location
information to the thunk, so the thunk simply inherits the random
location from the end of the function in front of it (the function
it's a thunk for). In two different compilation units compiled
with different options, this could result in two different locations
for the same thunk, and gold will give a false positive ODR
violation warning.

Second, if you try to compile with -S -dA, GCC crashes in final(),
where it's trying to build a mapping from insn to bb, because the
function has no cfg, and FOR_EACH_BB_REVERSE tries to dereference
cfun->cfg without checking for non-NULL.

I've fixed the first problem by adding a direct call to
(*debug_hooks->source_line) in assemble_thunk. This seems to work,
but I'm not at all sure if it's the right way to fix it. Advice
is appreciated.

I've fixed the second problem by adding a check for cfun->cfg != NULL
to guard the FOR_EACH_BB_REVERSE loop in final().

Do these changes look OK?

-cary

2012-08-03  Cary Coutant  <ccoutant@google.com>

	* gcc/cgraphunit.c (assemble_thunk): Add source line info.
	* gcc/final.c (final): Check for non-null cfg pointer.

Following up with a simple test case. If the patch looks OK, I'll work
this into a dejagnu test and add it to the patch.

class A
{
 public:
  A();
  virtual ~A();
 private:
  int i;
};

class B
{
 public:
  B();
  virtual ~B();
 private:
  int i;
};

class C : public A, public B
{
 public:
  C();
  virtual ~C();
};

C::C()
{
}

C::~C()
{
}

Compiled with -g -ffunction-sections, we should see something like
this for the thunks:

        .section        .text._ZThn16_N1CD1Ev,"ax",@progbits
        .globl  _ZThn16_N1CD1Ev
        .type   _ZThn16_N1CD1Ev, @function
_ZThn16_N1CD1Ev:
.LFB7:
        .cfi_startproc
        subq    $16, %rdi
        jmp     .LTHUNK0
        .cfi_endproc

and

        .section        .text._ZThn16_N1CD0Ev,"ax",@progbits
        .globl  _ZThn16_N1CD0Ev
        .type   _ZThn16_N1CD0Ev, @function
_ZThn16_N1CD0Ev:
.LFB8:
        .cfi_startproc
        subq    $16, %rdi
        jmp     .LTHUNK1
        .cfi_endproc

On Sat, Aug 4, 2012 at 1:38 AM, Cary Coutant <ccoutant@google.com> wrote:
> diff --git a/gcc/final.c b/gcc/final.c
> index 095d608..d22130f 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -1863,11 +1863,12 @@ final (rtx first, FILE *file, int optimize_p)
>        start_to_bb = XCNEWVEC (basic_block, bb_map_size);
>        end_to_bb = XCNEWVEC (basic_block, bb_map_size);
>
> -      FOR_EACH_BB_REVERSE (bb)
> -       {
> -         start_to_bb[INSN_UID (BB_HEAD (bb))] = bb;
> -         end_to_bb[INSN_UID (BB_END (bb))] = bb;
> -       }
> +      if (cfun->cfg != NULL)
> +       FOR_EACH_BB_REVERSE (bb)
> +         {
> +           start_to_bb[INSN_UID (BB_HEAD (bb))] = bb;
> +           end_to_bb[INSN_UID (BB_END (bb))] = bb;
> +         }
>      }
>
>    /* Output the insns.  */

Maybe check cfun->is_thunk instead? Everything else should have a cfg.

Ciao!
Steven

On 08/03/2012 04:38 PM, Cary Coutant wrote:
> 2012-08-03  Cary Coutant  <ccoutant@google.com>
> 
> 	* gcc/cgraphunit.c (assemble_thunk): Add source line info.
> 	* gcc/final.c (final): Check for non-null cfg pointer.

I'm uncomfortable with just the one call into the debug generator, outside of the other debug_hooks begin/end calls.

It'll obviously work for stabs, and probably work for sdb, due to how the debug info is represented.

But for dwarf2 it probably only works for selected targets.

For instance, !DWARF2_ASM_LINE_DEBUG_INFO requires a call to dwarf2out_finish in order to get anything emitted at all.  Some targets, like x86, use final_start_function + final_end_function in the output_mi_thunk hook, and that would take care of it.  However, x86-linux is also going to define DWARF2_ASM_LINE_DEBUG_INFO making both cases work.  And I'm guessing that's all you tested.

Try a target like arm-linux (which doesn't use final_end_function), and hack the generated auto-host.h so that HAVE_AS_DWARF2_DEBUG_LINE is undefined.


r~


PS: Yet Another Reason thunks should be represented as first-class citizens with a decl and other assorted paths through code emission done "properly".

> I'm uncomfortable with just the one call into the debug generator, outside of the other debug_hooks begin/end calls.
>
> It'll obviously work for stabs, and probably work for sdb, due to how the debug info is represented.
>
> But for dwarf2 it probably only works for selected targets.
>
> For instance, !DWARF2_ASM_LINE_DEBUG_INFO requires a call to dwarf2out_finish in order to get anything emitted at all.  Some targets, like x86, use final_start_function + final_end_function in the output_mi_thunk hook, and that would take care of it.  However, x86-linux is also going to define DWARF2_ASM_LINE_DEBUG_INFO making both cases work.  And I'm guessing that's all you tested.

Yep, that's all I tested. But even where output_mi_thunk doesn't call
final_start_function or final_end_function, dwarf2out_finish will
still be called, won't it?

> Try a target like arm-linux (which doesn't use final_end_function), and hack the generated auto-host.h so that HAVE_AS_DWARF2_DEBUG_LINE is undefined.

Trying arm-unknown-linux-gnueabi now...

-cary

Richard,

>> Try a target like arm-linux (which doesn't use final_end_function), and hack the generated auto-host.h so that HAVE_AS_DWARF2_DEBUG_LINE is undefined.
>
> Trying arm-unknown-linux-gnueabi now...

I just built an ARM compiler and tried it out on my testcase. It
generated this code for one of the thunks:

        .section        .text._ZThn8_N1CD1Ev,"ax",%progbits
        .align  2
        .global _ZThn8_N1CD1Ev
        .type   _ZThn8_N1CD1Ev, %function
_ZThn8_N1CD1Ev:
        .fnstart
        @ thunk.cc:25
.LM21:
        sub     r0, r0, #8
        b       .LTHUNK0
        .fnend
        .size   _ZThn8_N1CD1Ev, .-_ZThn8_N1CD1Ev

and further down, in the .debug_line section, this:

        .byte   0       @ set address *.LM21
        .uleb128 0x5
        .byte   0x2
        .4byte  .LM21
        .byte   0x2f    @ line 25

Same for the other thunk. I think that's all good.

Do you still have concerns about the patch?

-cary

On 08/06/2012 02:45 PM, Cary Coutant wrote:
> Do you still have concerns about the patch?

Nope.  I'd mis-remembered from whence things get finalized.

Revised patch is ok.


r~

> Revised patch is ok.

Thanks! Committed at r190190.

-cary