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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I think we're getting pretty close. I have a couple of extra comments inline, but they're all pretty minor, I hope. I'd just like to add some more tests, per our discussion on the IRC, and I have some doubts about what exactly is the test you've written actually testing, which I'd like to clarify...



================
Comment at: lldb/CMakeLists.txt:101
 
-  set(LLDB_TEST_DEPS lldb)
+  set(LLDB_TEST_DEPS lldb llvm-nm llvm-strip)
 
----------------
It looks like you're already adding these in `lldb/lit/CMakeLists.txt`. Does it need to be in both?


================
Comment at: lldb/include/lldb/Host/LZMA.h:24-25
+
+llvm::Error getUncompressedSize(llvm::ArrayRef<uint8_t> InputBuffer,
+                                uint64_t &uncompressedSize);
+
----------------
Now that the vector is auto-resized, do you think it's still useful to expose the `getUncompressedSize` function as public api? If yes, then please change it to return Expected<uint64_t>. (returning Expected would be nice even for a private api, but there it's less important...)


================
Comment at: lldb/lit/Breakpoint/Inputs/minidebuginfo-lib.h:1
+// This function will be embedded within the .dynsym section.
+int multiplyByThree(int num);
----------------
Hmm... so, what is the exact scenario you want to test here? This function will end up in the dynsym section of the main executable, but it will be an *undefined* symbol there. Undefined symbols are ignored in the gnu_debugdata logic. The symbol will be *defined* in the dynsym section of the shared library, but you're not debugdata-ing that one.

While this scenario might be interesting to test, I am not sure if that's the one you actually intended...

PS: If you want to create a file with both dynsym, and non-dynsym defined symbols, you don't have to resort to using shared libraries to achieve that. A suitable combination of -rdynamic and visibility("hidden") will achieve that too. For example the following line:
```
clang -x c - <<<'__attribute__((visibility("hidden"))) int in_symtab() { return 42; } int in_dynsym() { return 47; } int main() { return in_symtab()+in_dynsym(); }' -rdynamic
```
will generate a file where the in_symtab function will only be present in the symtab, and in_dynsym will be in the dynsym section (until you apply the debug-data magic, it will be in the symtab too)...


================
Comment at: lldb/lit/Breakpoint/minidebuginfo.test:32
+
+# RUN: llvm-objcopy -S --remove-section .gdb_index --remove-section .comment --keep-symbols=keep_symbols %t.debug %t.mini_debuginfo
+
----------------
Shouldn't this be `%t.keep_symbols`? Typo ?


================
Comment at: lldb/lit/lit.cfg.py:102
+
+if config.lldb_enable_lzma == "ON":
+    config.available_features.add('lzma')
----------------
you should apply `llvm_canonicalize_cmake_booleans` to the value of LLDB_ENABLE_LZMA at the cmake level. Otherwise, this will blow up if someone types `-DLLDB_ENABLE_LZMA=On`


================
Comment at: lldb/source/Host/common/LZMA.cpp:72-82
+  auto opts = lzma_stream_flags{};
+  if (InputBuffer.size() < LZMA_STREAM_HEADER_SIZE) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "size of xz-compressed blob (%lu bytes) is smaller than the "
+        "LZMA_STREAM_HEADER_SIZE (%lu bytes)",
+        InputBuffer.size(), LZMA_STREAM_HEADER_SIZE);
----------------
too much auto


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1886
+  section->GetSectionData(data);
+  llvm::ArrayRef<uint8_t> compressedData(data.GetDataStart(), data.GetByteSize());
+  llvm::SmallVector<uint8_t, 0> uncompressedData;
----------------
You don't need the temporary ArrayRef. You can just pass `data.GetData()` directly..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791