Page MenuHomePhabricator

[Debuginfo][Salvaging] (5/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).
Needs ReviewPublic

Authored by alok on Jul 18 2020, 5:09 PM.


This patch stems from D84112.
This patch includes improvement in debuginfo salvaging.

Diff Detail

Event Timeline

alok created this revision.Jul 18 2020, 5:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2020, 5:09 PM
jmorse added a subscriber: jmorse.Jul 30 2020, 5:58 AM

Could you elaborate on why module-level information needs to be preserved? As mentioned on llvm-dev, I think you might have missed a place where promotion from stack to SSA/values happens -- IMHO we shouldn't add module-level debuginfo-tracking unless it's absolutely necessary.


You'll need to use a value handle (WeakVH perhaps) to store Value references, to avoid storing pointers that become dangling pointers later. The comment should indicate what's being mapped -- I believe it's old alloca to the DILocalVariable, because insertVarAddressMap inserts the second operand of a dbg.declare?

In addition, it looks like salvageDebugInfoForDbgValues never dereferences the mapped value, so could this instead be a set instead of a map?


Does this make the debug-info metadata code in the Instruction destructor redundant?


The rest of Instruction.cpp is very detail-agnostic, and it seems out of place to check the DWARF version here (and presumably meaningless for CodeView). Could this be turned into a more generic compile option, or target option, so that it's less DWARF specific?

(It might be that LLVM doesn't support such a form of option).


Quite a lot of this function is now dedicated to "if we're an alloca being deleted, that had associated dbg.declares". IMO, it's clearer to put that code into a separate function.


We can't call clang from within llvm as it's not guaranteed to be built. Getting the unoptimised IR with -O0 -gdwarf-5 -emit-llvm -S -Xclang -disable-O0-optnone, and writing a normal lit test for that, would be best.

You might also want to put an end-to-end Dexter test into the debuginfo-tests.


This should be a positive test that the dbg.value points at a correct thing, not that it doesn't point at an undef. There are a variety of other Values it could point at by mistake.

alok updated this revision to Diff 304753.Nov 12 2020, 1:53 AM
alok retitled this revision from [Debuginfo] (6/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy). to [Debuginfo][Salvaging] (5/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy)..

Updated to re-base.

djtodoro added inline comments.

Can we leave this here in Local ?


nit: Preserve


why only for DWARF 5?
There is dw_op_gnu_implicit_pointer for the later version ?


Please add a comment describing what we are doing here.


Please use early continue here.

djtodoro added inline comments.Nov 12 2020, 2:43 AM

'later version' --> I meant earlier