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

[Pavel Labath via Phabricator via lldb-commits]

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