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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
