Page MenuHomePhabricator

[IR] Add DebugEntryValues pass
Needs ReviewPublic

Authored by milica-lazarevic on Dec 30 2021, 4:50 AM.

Details

Summary

This IR pass collects the function parameters and maps them to their entry values. If parameter's value remains unchanged we can restore it original value and make it available in the debugging process. This pass introduces the using of DW_OP_LLVM_entry_value operation on IR level. Pass has been added to the default<O2> pass pipeline, right after the DeadArgumentEliminationPass.

When a function inlining happens, there is no typical parameter passing. This situation is handled by deleting calls to llvm.dbg.value instructions which describe the location of the variable as an entry value.

For understanding motivation for introducing this kind of feature, consider an example below:

extern void fn1 ();
long int __attribute__((noinline)) fn2 (long int x)
{
  fn1();
  return 0;
}
void fn3 (){
  fn2(1);
}

Due to DeadArgumentEliminationPass, parameter x will be mark as undef and reported as <optimized_out> by the debugger. After applying DebugEntryValues pass, and with existence of corresponding DW_TAG_call_site_parameter, the parameter value will be available in the debugging process.

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

milica-lazarevic requested review of this revision.Dec 30 2021, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2021, 4:50 AM

This patch is a great use of entry values, however I think there's an awkward semantic gap caused by having it in a separate pass to DAE: any undef dbg.value can be promoted to being an entry value. When DAE runs and sets args to be undef we know for certain that the dbg.values being set to undef referred to arguments, but the inverse isn't true, any old undef dbg.value didn't necessarily refer to an argument.

It's probably OK if there's only one dbg.value, as mentioned in the code comments -- however, I don't believe the implementation in this patch counts to see that there's only one dbg.value, it just converts all undef dbg.values to entry values.

In comparison, if this were done in DAE there wouldn't be that uncertainty, plus I think we'd be able to support scenarios where there are other dbg.values of parameter variables, as we would know which dbg.values used to refer to the argument. (Unless I've missed something). I hate to be the reviewer who says "Why not do it completely differently", but wouldn't that be better?

I believe this code should ignore any functions marked "internal", because DAE can change the function signature of those functions, making the parameter-numbers in DILocalVariables not line up with the arguments. See: llvm/test/Transforms/DeadArgElim/dbginfo.ll .

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
393–396

What's the motivation behind this change -- it's not covered by any of the tests changed / added, no?

llvm/lib/Transforms/Utils/DebugEntryValues.cpp
40

Would just the idx < F.arg_size() clause be sufficient?

llvm/lib/Transforms/Utils/InlineFunction.cpp
1757

Am I right in thinking this RAUWs a dbg.value instruction? I don't believe there should be any users of the dbg.value, as it doesn't produce a Value.

llvm/test/Transforms/DebugEntryValues/no-param-change.ll
27–30

Any un-necessary attributes should be dropped to reduce future maintenance

ormris removed a subscriber: ormris.Jan 18 2022, 10:06 AM