[lldb][ELF] Read symbols from .gnu_debugdata sect.

[kwk]

Summary

If the .symtab section is stripped from the binary it might be that
there's a .gnu_debugdata section which contains a smaller .symtab in
order to provide enough information to create a backtrace with function
names or to set and hit a breakpoint on a function name.

This change looks for a .gnu_debugdata section in the ELF object file.
The .gnu_debugdata section contains a xz-compressed ELF file with a
.symtab section inside. Symbols from that compressed .symtab section
are merged with the main object file's .dynsym symbols (if any).
In addition we always load the .dynsym even if there's a .symtab
section.

For example, the Fedora and RHEL operating systems strip their binaries
but keep a .gnu_debugdata section. While gdb already can read this
section, LLDB until this patch couldn't. To test this patch on a
Fedora or RHEL operating system, try to set a breakpoint on the "help"
symbol in the "zip" binary. Before this patch, only GDB can set this
breakpoint; now LLDB also can do so without installing extra debug
symbols:

lldb /usr/bin/zip -b -o "b help" -o "r" -o "bt" -- -h

The above line runs LLDB in batch mode and on the "/usr/bin/zip -h"
target:

(lldb) target create "/usr/bin/zip"
Current executable set to '/usr/bin/zip' (x86_64).
(lldb) settings set -- target.run-args  "-h"

Before the program starts, we set a breakpoint on the "help" symbol:

(lldb) b help
Breakpoint 1: where = zip`help, address = 0x00000000004093b0

Once the program is run and has hit the breakpoint we ask for a
backtrace:

(lldb) r
Process 10073 stopped
* thread #1, name = 'zip', stop reason = breakpoint 1.1
    frame #0: 0x00000000004093b0 zip`help
zip`help:
->  0x4093b0 <+0>:  pushq  %r12
    0x4093b2 <+2>:  movq   0x2af5f(%rip), %rsi       ;  + 4056
    0x4093b9 <+9>:  movl   $0x1, %edi
    0x4093be <+14>: xorl   %eax, %eax

Process 10073 launched: '/usr/bin/zip' (x86_64)
(lldb) bt
* thread #1, name = 'zip', stop reason = breakpoint 1.1
  * frame #0: 0x00000000004093b0 zip`help
    frame #1: 0x0000000000403970 zip`main + 3248
    frame #2: 0x00007ffff7d8bf33 libc.so.6`__libc_start_main + 243
    frame #3: 0x0000000000408cee zip`_start + 46

In order to support the .gnu_debugdata section, one has to have LZMA
development headers installed. The CMake section, that controls this
part looks for the LZMA headers and enables .gnu_debugdata support by
default if they are found; otherwise or if explicitly requested, the
minidebuginfo support is disabled.

GDB supports the "mini debuginfo" section .gnu_debugdata since v7.6
(2013).

[kwk]

lldb/include/lldb/Host/LZMA.h

25–26

I've changed the syntax as you suggested. I kept the function there because I find it easier to maintain this way.

lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml

1 ↗

(On Diff #219038)

I've move all to lit/Modules/ELF.

13 ↗

(On Diff #219038)

I use -x and not -X. See lldb --help:

-x Alias for --no-lldbinit
-X Alias for --no-use-color

lldb/lit/Breakpoint/minidebuginfo-no-lzma.yaml

22 ↗

(On Diff #219038)

I kept it the same for the like the other files so you can text-diff it to find out that only one byte was modified. I thought this is a neat idea in order to actually error out upon decompression and not on file header for example. That being said what do you think about adding tests for the different errors that can occur during the whole inspection (e.g. in getUncompressedSize).

lldb/source/Host/common/LZMA.cpp

82–83

All done now.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1850

Done.

1886

Sorry for missing this. Done now.

[labath]

lldb/include/lldb/Host/LZMA.h

25–26

Ok. I am fine with that.

lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml

13 ↗

(On Diff #219038)

Ah, my bad. In that case you definitely *can* remove it because %lldb does add --no-lldbinit.

lldb/lit/Breakpoint/minidebuginfo-no-lzma.yaml

22 ↗

(On Diff #219038)

Ok. That's fine with me.
I am definitely not *against* adding more tests. :) I am not going to ask you to put more in because this patch already has more tests than most lldb patches, but I you want to do that, then by all means, go ahead. Things like these would be probably better off as a (c++) unit test, as they're really testing the details of the lzma handling code. The elf code doesn't really care about these details -- it just wants to know whether the decompression was successful or not.

[kwk]

labath, please see my answer to your question and maybe help me out why you think I don't test what I want to test.

lldb/lit/Breakpoint/minidebuginfo-set-and-hit-breakpoint.test

50 ↗

(On Diff #219038)

labath, I'd like to test the following scenario: Set a breakpoint on a symbol (multiplyByThree) in the .dynsym section and one on a symbol (multiplyByFour) from the .symtab that's embedded within the .gnu_debugdata section. I have no clue how to get those symbols without using a shared lib. Do you?

[kwk]

lldb/lit/Breakpoint/minidebuginfo-set-and-hit-breakpoint.test

50 ↗

(On Diff #219038)

I found a better and simpler way thanks to the help from @jankratochvil . Will implement it soon.

[MaskRay]

lldb/source/Host/common/LZMA.cpp

125

empty

130

if (auto err = ...)

return err;

137

is more common

139

stray inpos = 0;

[kwk]

• Remove -x from %lldb because that expands to the command which already includes it
• Improve and fix the minidebuginfo-set-and-hit-breakpoint.test

[kwk]

@labath I have greatly improved the set and hit breakpoint test. Thank you again @jankratochvil for bringing my attention to -Wl,--dynamic-list. That helped to put one symbol in .dynsym explicitly. I will see if I can utilize that knowledge to separate out the patch this comment of yours @labath :

Let's put this bit into a separate patch. As I said over IRC, I view this bit as functionally independent of the debugdata stuff (it's definitely independent in it's current form, as it will kick in for non-debugdata files too, which I think is fine).

[kwk]

Rebased on master to include changes from D67390

[kwk]

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

2715–2740

Done in D67390.

[labath]

This looks great now. Thank you for your patience. Just make sure to deal with the breakages from the other patch first. If the fix is non trivial, or if it's going to take a while, then please revert to unbreak the bots.

lldb/lit/Modules/ELF/minidebuginfo-corrupt-xz.yaml

2

system-linux shouldn't be required here. One of the reasons I wanted to have these tests is that they can run on any system, not just on linux.

lldb/lit/Modules/ELF/minidebuginfo-find-symbols.yaml

2

No system-linux

lldb/lit/Modules/ELF/minidebuginfo-no-lzma.yaml

2

no system-linux

lldb/source/Host/common/LZMA.cpp

130

I have a feeling this pattern is actually more common in lldb. I tend to use this one, but I am fine with either.

[kwk]

• Addressed review comments

[kwk]

This comment was removed by kwk.

lldb/source/Host/common/LZMA.cpp

130

So what now? I had getCompressedSize return an error before and changed it upon request to expected. I can live with both but like the expected thing a bit better in terms of: this goes in and this comes out mentality.

[kwk]

lldb/lit/Modules/ELF/minidebuginfo-corrupt-xz.yaml

2

@labath what about the other file minidebuginfo-set-and-hit-breakpoint.test. Remove system-linux from there as well?

[labath]

lldb/lit/Modules/ELF/minidebuginfo-corrupt-xz.yaml

2

You need to restrict that test somehow, because it only has a chance for working on elf platforms (as you're compiling binaries for the host and running them). It probably *should* work on all elf platforms, but we currently don't have a good way of expressing that and we're just restricting to system-linux currently.

If you feel adventurous you can try introducing a system-elf feature (or something), and then changing this test (and possibly others) to use that instead, but please do that as a separate patch.

lldb/source/Host/common/LZMA.cpp

130

My interpretation of @MaskRay's comment was that he wanted you do write something like

if (auto err = uncompressedSize.takeError())
  return err;

That is orthogonal to what type is returned by getCompressedSize, and is indeed a more common pattern in at least some parts of llvm. I am personally fine with both.

[kwk]

• Change logic and comment for when .dynsym is parsed
• Use different pattern for error checking
• use less auto
• Remove required system-linux test feature from LZMA tests

[kwk]

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

2715–2740

We agreed to not have this other patch. See D67390#1691424.

[kwk]

Tests did pass so this change is ready for review @labath @jankratochvil .

[jankratochvil]

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1854

I miss here comparing that the sections inside .gnu_debugdata file match sections in the outer file:

jankratochvil: "hm.... I was under the impression that section number should remain the same in regular and split debug files" - maybe you are right but then we could just also compare the two section tables and if they differ then reject whole .gnu_debugdata.  And then we do not have to compare section names but it is enough to compare section indexes.
labath: "right but then we could just also compare the two section tables and if they differ then reject whole .gnu_debugdata."
labath: I would like that ^

Also I do not see here handling .strtab, why does it work without moving also .strtab?

[labath]

Ok, let's try this once more. I took another look at the patch, and I have some more tiny comments, but nothing too major.

I'm sorry this took so long. When the topic of parsing both symbol sections came up, I was happy that it was implemented in a way that applies outside of minidebuginfo too, because it seemed like it could be a useful source of additional information. However, that turned into a huge rabbit hole, which isn't really relevant for the thing you're trying to accomplish. If we've managed to come this far without needing to consult both symbol tables, I think we'll manage to wait a couple years longer.

lldb/lit/Modules/ELF/minidebuginfo-no-lzma.yaml

5

s/a. /a ./

lldb/source/Host/common/LZMA.cpp

2

Fix header.

70–73

This is redundant, as you're comparing the size against LZMA_STREAM_HEADER_SIZE anyway.

85

Maybe InputBuffer.take_back(LZMA_STREAM_HEADER_SIZE).data() ?

105–106

same here.

126–129

This is redundant, as getUncompressedSize checks that anyway.

138

= UINT64_MAX;. This looks too much like a function declaration, and is not consistent with the code below.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1854

I'd leave that out for now. This is similar stuff to what is already being done in SymbolVendorELF, which should also probably check for section matches, and would be best handled together.

The reason that this works without moving .strtab is because the parsing is still done within the context of the owning object file. This whole section merging business is a pretty big mess, and in dire need of some TLC, but @kwk is not making that worse, so I wouldn't want to deal with that here...

2725

You can add that this is because minidebuginfo normally removes the symtab symbols which have their matching dynsym counterparts.

[kwk]

• Fix typo: s/a. /a ./
• Fix comment header
• Remove redundant input buffer check
• Remove another redundant input buffer check
• Change variable initialization to assign
• added comment about minidebuginfo

[kwk]

@labath I've addressed all you comments. Thank you for answering @jankratochvil's question as well. If this patch is good to go now, please give a thumbs up.

lldb/source/Host/common/LZMA.cpp

85

take_back:

Return a copy of *this with only the last \p N elements.

I mean LZMA_STREAM_HEADER_SIZE is only 12 but do we really need it?

105–106

Same answer here.

[kwk]

@labath ah, I've just seen that you did accept the revision already. Will merge it then after running tests once more.

[kwk]

Rebased onto current master

[kwk]

@labath as much as I would love to merge this patch I get errors from these tests that I didn't see before. They only appear after rebasing onto master:

Failing Tests (4):
    lldb-Suite :: commands/process/attach/TestProcessAttach.py
    lldb-Suite :: python_api/hello_world/TestHelloWorld.py
    lldb-Suite :: tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py
    lldb-Suite :: tools/lldb-vscode/attach/TestVSCode_attach.py

I also get the same errors on current master .(revision 8815be04ec1f333564591d9593735f22efa9bee5) without my patch applied. But before I merrge my patch I'd like to see master in a better shape, no?

[jankratochvil]

In D66791#1696340, @kwk wrote:

lldb-Suite :: commands/process/attach/TestProcessAttach.py
lldb-Suite :: tools/lldb-vscode/attach/TestVSCode_attach.py

These are by: D68289

lldb-Suite :: python_api/hello_world/TestHelloWorld.py
lldb-Suite :: tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py

The Fedora buildbot shows different two ones:

lldb-Suite :: commands/gui/basic/TestGuiBasic.py
lldb-Suite :: python_api/hello_world/TestHelloWorld.py

Although I do not have those reproducible on my local Fedora 30 host.

[kwk]

rebased

[kwk]

Closed by 2c082b48274fcba62bf9b3acb63075aedcc7a976.

[labath]

lldb/source/Host/common/LZMA.cpp

85

Yeah, that documentation is somewhat misleading. If you look at the implementation you'll see that there's no copying involved (ArrayRefs never copy anything -- that's their whole point of existence). So, I still think that this is better/more readable with take_back.