[Lldb-commits] [PATCH] D42563: [lldb] attempt to fix DIERef::GetUID

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap created this revision.
alexshap added reviewers: tberghammer, labath, clayborg.
Herald added subscribers: llvm-commits, JDevlieghere, aprantl.

A small test where the issue is easy to reproduce:

a.cpp:

  int f() {
    return 1;
  }

b.cpp:

  extern int f();
  void g() {
     int y = 14;
     int x = f();
  }
  int main() {
     g();
     return 0;
  }

g++ -O0 -g -gsplit-dwarf -c a.cpp -o a.o // with fission
g++ -O0 -g -c b.cpp -o b.o                      // without fission
g++ a.o b.o -o main.exe                           // the order matters

run lldb
br set --name g  // WORKS FINE
run                      // WORKS FINE
frame variable    // DOESN'T WORK

So the problem is the following:
on Linux dwarf->GetID() returns 0 (unless dwarf is an instance of SymbolFileDWARFDwo),

> the higher 32 bits of  the returned by DIERef::GetUID(SymbolFileDWARF *dwarf) uid do not encode cu_offset.
============================================================================================================

What happens next - in the constructor of DIERef the incorrect value of cu_offset is extracted from uid and propagates further.
For example, inside SymbolFileDWARF::ParseVariablesForContext there is a call 
DWARFDIE function_die = info->GetDIE(DIERef(sc.function->GetID(), this))

- and because of the "broken" cu_offset (inside DIERef) the method GetDIE returns an invalid DWARFDIE.

I don't like the proposed fix myself, maybe you have better ideas how to address the issue. 
Partially the complexity stems from the fact that DIERefs are used in many places => they need to contain the correct cu_offset.
Next, it's important to keep in mind that one file may contain several compilation units => fixing SymbolFileDWARF::GetID doesn't seem to be an option. Anyway - suggestions are very welcome, maybe I'm missing smth.

Test plan: make check-lldb


Repository:
  rL LLVM

https://reviews.llvm.org/D42563

Files:
  source/Plugins/SymbolFile/DWARF/DIERef.cpp


Index: source/Plugins/SymbolFile/DWARF/DIERef.cpp
===================================================================
--- source/Plugins/SymbolFile/DWARF/DIERef.cpp
+++ source/Plugins/SymbolFile/DWARF/DIERef.cpp
@@ -13,6 +13,7 @@
 #include "DWARFFormValue.h"
 #include "SymbolFileDWARF.h"
 #include "SymbolFileDWARFDebugMap.h"
+#include "lldb/Symbol/ObjectFile.h"
 
 DIERef::DIERef()
     : cu_offset(DW_INVALID_OFFSET), die_offset(DW_INVALID_OFFSET) {}
@@ -57,16 +58,21 @@
 }
 
 lldb::user_id_t DIERef::GetUID(SymbolFileDWARF *dwarf) const {
-  //----------------------------------------------------------------------
-  // Each SymbolFileDWARF will set its ID to what is expected.
-  //
-  // SymbolFileDWARF, when used for DWARF with .o files on MacOSX, has the
-  // ID set to the compile unit index.
-  //
-  // SymbolFileDWARFDwo sets the ID to the compile unit offset.
-  //----------------------------------------------------------------------
-  if (dwarf && die_offset != DW_INVALID_OFFSET)
-    return dwarf->GetID() | die_offset;
-  else
+  if (!dwarf || die_offset == DW_INVALID_OFFSET)
     return LLDB_INVALID_UID;
+
+  // For ELF targets the higher 32 bits of the returned id encode cu_offset
+  // which is used, for example, by DIERef.
+  lldb_private::ArchSpec arch;
+  if (dwarf->GetObjectFile()->GetArchitecture(arch) &&
+      arch.GetTriple().isOSBinFormatELF()) {
+    // For SymbolFileDWARFDwo/SymbolFileDWARFDwoDwp
+    // the offset of the base compilation unit should be used.
+    uint64_t offset =
+      dwarf->GetBaseCompileUnit() ? dwarf->GetBaseCompileUnit()->GetOffset() :
+      cu_offset;
+    return (offset << 32) | die_offset;
+  }
+
+  return dwarf->GetID() | die_offset;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42563.131539.patch
Type: text/x-patch
Size: 1716 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180126/717d0520/attachment.bin>