Page MenuHomePhabricator

[DebugInfo] Describe call site values for chains of expression producing instrs
ClosedPublic

Authored by dstenb on Feb 24 2020, 2:38 AM.

Details

Summary

If the describeLoadedValue() hook produced a DIExpression when
describing a instruction, and it was not possible to emit a call site
entry directly (the value operand was not an immediate nor a preserved
register), then that described value could not be inserted into the
worklist, and would instead be dropped, meaning that the parameter's
call site value couldn't be described.

This patch extends the worklist so that each entry has an DIExpression
that is built up when iterating through the instructions.

This allows us to describe instruction chains like this:

$reg0 = mv $fp
$reg0 = add $reg0, offset
call @call_with_offseted_fp

Since DW_OP_LLVM_entry_value operations can't be combined with any other
expression, such call site entries will not be emitted. I have added a
test, dbgcall-site-expr-entry-value.mir, which verifies that we don't
assert or emit broken DWARF in such cases.

Diff Detail

Event Timeline

dstenb created this revision.Feb 24 2020, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 2:38 AM
dstenb marked an inline comment as done.Feb 24 2020, 2:40 AM
dstenb added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
709

Perhaps this lambda and addToWorklist should be separate functions now that they have grown quite a bit?

This makes sense to me.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
709

Hmm, I think so.

And definitely, I don't like catching all data with [&] when using lambdas (in spite of the fact that probably I put that here :)), but if we are refactoring that into a function, it is not important.

dstenb marked an inline comment as done.Feb 24 2020, 7:42 AM
dstenb added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
709

I split that change out to D75050.

Thanks for this!

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-chain.mir
117

Have you tried debugging this with GDB/LLDB? I'm wondering if debuggers are missing the support for that.

These expressions seems valid to me, but I'm not sure even GCC generate call values with "combined" expressions so far.

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-entry-value.mir
32

Can we delete these?

dstenb marked 2 inline comments as done.Feb 25 2020, 6:38 AM
dstenb added inline comments.
llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-chain.mir
117

I unfortunately don't have an AArch64 setup I can run GDB on, so I have mostly been testing on our downstream target.

I now also tried this on x86-64 by using the following C code:

extern int call(int);

int foo = 1000;

int main() {
  int local = foo + 789;
  call(local);
  return local;
}

in which I changed the parameter setup code to the following (and described ADD32ri in isAddImmediate()):

renamable $ebx = MOV32rm $rip, 1, $noreg, @foo, $noreg, debug-location !25 :: (dereferenceable load 4 from @foo, !tbaa !21)
$edi = MOV32rr $ebx                                                                                                                                                                                                                                                                                                        
renamable $edi = ADD32ri $edi, 700, implicit-def $eflags
renamable $edi = ADD32ri $edi, 80, implicit-def $eflags
renamable $edi = ADD32ri $edi, 9, implicit-def $eflags

resulting in the following expression:

DW_AT_location      (DW_OP_reg5 RDI)
DW_AT_GNU_call_site_value   (DW_OP_breg3 RBX+700, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_plus_uconst 0x50, DW_OP_plus_uconst 0x9)

which prints fine in LLDB trunk:

   3   	int call(int param) {
   4   	  side_effect = param;
   5   	  __asm volatile("" : : : "rdi"); // clobber register.
-> 6   	  return 0; // print param here.
   7   	}
(lldb) print param
(int) $0 = 1789

and GDB 8.2.1:

(gdb) bt
#0  call (param=param@entry=1789) at call.c:6
#1  0x000000000020110a in main () at main.c:7
llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-entry-value.mir
32

Ah, yes! I'll remove those.

This looks good to me with the comments addressed. Thanks!

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-chain.mir
117

Nice!

I think this should have good impact on the number of call_site_params (call site param DIEs field from llvm-dwarfdump stats output) in general.

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-entry-value.mir
57

nit: We can attach to only !17, and get rid of the !18 and !19.

dstenb updated this revision to Diff 246480.Feb 25 2020, 8:29 AM

Address comments.

djtodoro accepted this revision.Feb 26 2020, 12:47 AM

This looks good to me. Thanks!

(Out of the topic of this patch) I'm thinking of the fact all this "entry-values and call-site-values" related work is getting bigger, so it might be worth of refactoring all of this in a class or a compile unit, but we can discuss about it later.

This revision is now accepted and ready to land.Feb 26 2020, 12:47 AM
dstenb added a comment.EditedFeb 26 2020, 8:32 AM

This looks good to me. Thanks!

Thanks for the review! I'm planning on landing this tomorrow unless there are any more comments.

(Out of the topic of this patch) I'm thinking of the fact all this "entry-values and call-site-values" related work is getting bigger, so it might be worth of refactoring all of this in a class or a compile unit, but we can discuss about it later.

That sounds like a good idea.

EDIT: "commits" -> "comments"

vsk added a comment.Feb 26 2020, 8:43 AM

Thanks, this is really nice work.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
605

I think it'd aid readability to assign I.first->second to some local reference variable.

618

Does this implicitly assert !Expr->isEntryValue()?

dstenb marked an inline comment as done.Feb 26 2020, 8:57 AM
dstenb added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
618

Ah, yes. I did not consider that any describeLoadedValue implementation would may want to emit entry value expressions, but I guess it's fair for them to be able to do that.

This will bail out later due to a failed isValid() check, but I can add an explicit assert here instead.

dstenb updated this revision to Diff 246775.Feb 26 2020, 9:50 AM
dstenb marked 3 inline comments as done.

Address review comments.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
618

I added isValid() asserts to both places where the debug expressions are combined.

vsk accepted this revision.Feb 26 2020, 10:21 AM

LGTM

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-expr-chain.mir
104

nit, instruction per

108

Nice!

dstenb set the repository for this revision to rG LLVM Github Monorepo.Feb 27 2020, 5:37 AM
dstenb closed this revision.Feb 27 2020, 5:49 AM
dstenb marked an inline comment as done.

Oh, it seems that I missed to specify the repository for this patch. This was pushed as:

commit 6d857166d218ccedfff3b177a4489420e3bdd603 (HEAD -> master, origin/master, origin/HEAD)
Author: David Stenberg <david.stenberg@ericsson.com>
Date:   Thu Feb 27 09:54:37 2020 +0100

    [DebugInfo] Describe call site values for chains of expression producing instrs
    
    Summary:
    If the describeLoadedValue() hook produced a DIExpression when
    describing a instruction, and it was not possible to emit a call site
    entry directly (the value operand was not an immediate nor a preserved
    register), then that described value could not be inserted into the
    worklist, and would instead be dropped, meaning that the parameter's
    call site value couldn't be described.
    
    This patch extends the worklist so that each entry has an DIExpression
    that is built up when iterating through the instructions.
    
    This allows us to describe instruction chains like this:
    
      $reg0 = mv $fp
      $reg0 = add $reg0, offset
      call @call_with_offseted_fp
    
    Since DW_OP_LLVM_entry_value operations can't be combined with any other
    expression, such call site entries will not be emitted. I have added a
    test, dbgcall-site-expr-entry-value.mir, which verifies that we don't
    assert or emit broken DWARF in such cases.
    
    Reviewers: djtodoro, aprantl, vsk
    
    Reviewed By: djtodoro, vsk
    
    Subscribers: hiraditya, llvm-commits
    
    Tags: #debug-info, #llvm
    
    Differential Revision: https://reviews.llvm.org/D75036

I have also pushed a couple of patches today, but it was not closed automatically. It says "Still importing.." (rG6d857166d218ccedfff3b177a4489420e3bdd603)