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

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

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

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...

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); }

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

if (m_first_die) {
       lldbassert(m_first_die == m_die_array.front());
     }
     m_first_die = m_die_array.front();
   }
 
   m_die_array.shrink_to_fit();
 
   if (m_dwo_symbol_file) {
+    // 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 just delete these extra DIEs
+    // (if any) and reclaim some space.
+    m_die_array.resize(1);
+    m_die_array.shrink_to_fit();
+    m_die_array[0].SetHasChildren(false);
+
     DWARFUnit *dwo_cu = m_dwo_symbol_file->GetCompileUnit();
     dwo_cu->ExtractDIEsIfNeeded();
   }
 }
 
 // This is used when a split dwarf is enabled.
 // A skeleton compilation unit may contain the DW_AT_str_offsets_base attribute
 // that points to the first string offset of the CU contribution to the