labath added a comment.
Looks mostly fine, but you'll need to split this patch up. You'll want a
separate review (and test) for the yaml2obj changes. Also, the lzma code should
not live inside ObjectFileELF. I'd put it somewhere under `lldb/Host/LZMA.h`.
For how to wrap lzma, I'd suggest looking at `llvm/Support/Compression.h` for
inspiration.
================
Comment at: lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp:79
+ // If there's none in the orignal object file, we add it to it.
+ if (auto gdd_obj_file =
+ obj_file->GetGnuDebugDataObjectFile()) {
----------------
Wouldn't it be better to first check for the external file, and then fall back
to gnu_debugdata, as the external file will likely have more complete debug
info?
================
Comment at: lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp:62
lldb_private::Stream *feedback_strm) {
+ (void)feedback_strm;
+
----------------
You seem to have a particularly fussy compiler. I don't remember ever needing
to mark function arguments (not locals) as unused in the default llvm config.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:320
+
+ bool HasSymbolsEntryInYAML() const { return Symbols.hasValue(); }
};
----------------
This would be lower camel case in llvm style. (But honestly, I'm not sure if
this function adds any value).
================
Comment at: llvm/include/llvm/Support/MathExtras.h:271
unsigned char out[sizeof(Val)];
- std::memcpy(in, &Val, sizeof(Val));
+ memcpy(in, &Val, sizeof(Val));
for (unsigned i = 0; i < sizeof(Val); ++i)
----------------
Huh?
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:201
+ }
if (!Doc.DynamicSymbols.empty())
ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
----------------
So, the .dynsym section is emitted only if the yaml table is non-empty, but now
.symtab will be emitted only when the yaml key is present? I think the .symtab
behavior is more reasonable (because then you are actually able to generate an
*empty* .symtab if needed), but consistency is important to. I think we'll need
to discuss this on the yaml patch.
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
