Page MenuHomePhabricator

Ignore DIEs in the skeleton unit in a DWO scenario
ClosedPublic

Authored by labath on Jun 4 2019, 4:37 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 4 2019, 4:37 AM
clayborg added inline comments.Jun 4 2019, 7:02 AM
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 added inline comments.Jun 4 2019, 7:49 AM
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 updated this revision to Diff 202986.Jun 4 2019, 10:56 AM
  • avoid parsing the extra DIEs in the first place
labath marked an inline comment as done.Jun 4 2019, 10:59 AM
labath added inline comments.
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.

clayborg accepted this revision.Jun 4 2019, 1:54 PM
This revision is now accepted and ready to land.Jun 4 2019, 1:54 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 12:28 AM
kwk added a subscriber: kwk.Jun 11 2019, 12:49 AM

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