Ignore DIEs in the skeleton unit in a DWO scenario

[labath]

Summary

r362103 exposed a bug, where we could read incorrect data if a skeleton
unit contained more than the single unit DIE. Clang emits these kinds of
units with -fsplit-dwarf-inlining (which is also the default).

Changing lldb to handle these DIEs is nontrivial, as we'd have to change
the UID encoding logic to be able to reference these DIEs, and fix up
various places which are assuming that all DIEs come from the separate
compile unit.

However, it turns out this is not necessary, as the DWO unit contains
all the information that the skeleton unit does. So, this patch just
deletes any extra DIEs which are present in the skeleton unit, and
enforces the invariant that the rest of the code is already operating
under.

This patch fixes a couple of existing tests, but I've also included a
simpler test which does not depend on execution of binaries, and would
have helped us in catching this sooner.

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

243–251 ↗

(On Diff #202906)

Can we detect and just not parse in the first place so we don't parse then throw away?

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

243–251 ↗

(On Diff #202906)

Also, what if we have one DWO file that isn't there where we don't delete these DIEs, followed by one that is. Won't we run into lldb::user_id_t collisions in this case? Should we just never parse DWO DIEs other than the first one to avoid this?

[labath]

• avoid parsing the extra DIEs in the first place

[labath]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

243–251 ↗

(On Diff #202906)

I've changed the parsing logic to bail out earlier.

There won't be any UID collisions if only some of the dwo files are missing, because we will, for all intents and purposes, treat a compile with a missing dwo file the same way as a regular (non-dwo) compile unit. And we already support mixing dwo and non-dwo compile units in a single file.

[kwk]

This patch fixes a couple of existing tests,

@labath can you please tell which tests are fixed by this commit exactly? I try to reproduce an issue with a test that (potentially under load) used tp sometimes hang. Now it does no longer do that and I wonder if your change has anything to do with that.

Here's the test call I'm talking about:

cd ~/llvm/lldb/test/
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:~/llvm-builds/relwithdebinfo-ninja-clang-gold-ccache-distcc/lib python dotest.py \
  -v \
  --executable ~/llvm-builds/relwithdebinfo-ninja-clang-gold-ccache-distcc/bin/lldb \
  -f TestVSCode_setBreakpoints.test_functionality \
  ../../lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint

[labath]

In D62852#1537529, @kwk wrote:

This patch fixes a couple of existing tests,

@labath can you please tell which tests are fixed by this commit exactly? I try to reproduce an issue with a test that (potentially under load) used tp sometimes hang. Now it does no longer do that and I wonder if your change has anything to do with that.

Here's the test call I'm talking about:

cd ~/llvm/lldb/test/
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:~/llvm-builds/relwithdebinfo-ninja-clang-gold-ccache-distcc/lib python dotest.py \
  -v \
  --executable ~/llvm-builds/relwithdebinfo-ninja-clang-gold-ccache-distcc/bin/lldb \
  -f TestVSCode_setBreakpoints.test_functionality \
  ../../lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint

I don't remember which tests exactly were broken by this, but I know that all of them were using __attribute__((always_inline)), and only the dwo variant was broken.

TestVSCode_setBreakpoints does not fit either of the two criteria, and was flaky even before the change which "broke" the dwo+inline combo, so I would be very surprised if this patch had any effect on that test...

lldb/trunk/lit/SymbolFile/DWARF/split-dwarf-inlining.cpp

+// RUN: %clangxx -target x86_64-pc-linux -gsplit-dwarf -fsplit-dwarf-inlining \
+// RUN:   -c %s -o %t
+// RUN: %lldb %t -o "breakpoint set -n foo" -b | FileCheck %s
+
+// CHECK: Breakpoint 1: 2 locations
+
+__attribute__((always_inline)) int foo(int x) { return x; }
+int bar(int x) { return foo(x); }

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

if (depth == 0) {
       // compile unit DIE. The compile unit DIE is always the first entry, so
       // if our size is 1, then we are adding the first compile unit child
       // DIE and should reserve the memory.
       m_die_array.reserve(GetDebugInfoSize() / 24);
       m_die_array.push_back(die);
 
       if (!m_first_die)
         AddUnitDIE(m_die_array.front());
+
+      // With -fsplit-dwarf-inlining, clang will emit non-empty skeleton compile
+      // units. We are not able to access these DIE *and* the dwo file
+      // simultaneously. We also don't need to do that as the dwo file will
+      // contain a superset of information. So, we don't even attempt to parse
+      // any remaining DIEs.
+      if (m_dwo_symbol_file) {
+        m_die_array.front().SetHasChildren(false);
+        break;
+      }
+
     } else {
       if (null_die) {
         if (prev_die_had_children) {
           // This will only happen if a DIE says is has children but all it
           // contains is a NULL tag. Since we are removing the NULL DIEs from
           // the list (saves up to 25% in C++ code), we need a way to let the
           // DIE know that it actually doesn't have children.
           if (!m_die_array.empty())