[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

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.



================
Comment at: lldb/lit/Modules/ELF/minidebuginfo-no-lzma.yaml:4
+# This test checks that a warning is printed when we're trying
+# to decompress a. gnu_debugdata section when no LZMA support was compiled in.
+
----------------
s/a. /a ./


================
Comment at: lldb/source/Host/common/LZMA.cpp:1
+//===--- Compression.cpp - Compression implementation ---------------------===//
+//
----------------
Fix header.


================
Comment at: lldb/source/Host/common/LZMA.cpp:69-72
+  if (InputBuffer.size() == 0)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "size of xz-compressed blob cannot be 0");
+
----------------
This is redundant, as you're comparing the size against LZMA_STREAM_HEADER_SIZE anyway.


================
Comment at: lldb/source/Host/common/LZMA.cpp:84
+  lzma_ret xzerr = lzma_stream_footer_decode(
+      &opts, InputBuffer.data() + InputBuffer.size() - LZMA_STREAM_HEADER_SIZE);
+  if (xzerr != LZMA_OK) {
----------------
Maybe `InputBuffer.take_back(LZMA_STREAM_HEADER_SIZE).data()` ?


================
Comment at: lldb/source/Host/common/LZMA.cpp:104-105
+      lzma_index_buffer_decode(&xzindex, &memlimit, nullptr,
+                               InputBuffer.data() + InputBuffer.size() -
+                                   LZMA_STREAM_HEADER_SIZE - opts.backward_size,
+                               &inpos, InputBuffer.size());
----------------
same here.


================
Comment at: lldb/source/Host/common/LZMA.cpp:125-128
+  if (InputBuffer.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "size of xz-compressed blob cannot be 0");
+
----------------
This is redundant, as getUncompressedSize checks that anyway.


================
Comment at: lldb/source/Host/common/LZMA.cpp:137
+  // Decompress xz buffer to buffer.
+  uint64_t memlimit(UINT64_MAX);
+  size_t inpos = 0;
----------------
` = UINT64_MAX;`. This looks too much like a function declaration, and is not consistent with the code below.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1854
+              gdd_objfile_section_list->FindSectionByType(
+                  eSectionTypeELFSymbolTable, true)) {
+        SectionSP module_section_sp = unified_section_list.FindSectionByType(
----------------
jankratochvil wrote:
> 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`?
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...


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2725
+    // minidebuginfo case we parse .dynsym when there's a .gnu_debuginfo
+    // section, nomatter if .symtab was already parsed or not.
+    if (!symtab ||
----------------
You can add that this is because minidebuginfo normally removes the symtab symbols which have their matching dynsym counterparts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66791/new/

https://reviews.llvm.org/D66791