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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added inline comments.


================
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()) {
----------------
labath wrote:
> 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?
My idea was to load whatever symbols we can get and let it be overwritten by the the more concrete ones that might come later. Changing the logic requires to your suggesttion would require a bit more effort in that I cannot simply leave the `return nullptr` expressions untouched. 


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:320
+
+  bool HasSymbolsEntryInYAML() const { return Symbols.hasValue(); }
 };
----------------
labath wrote:
> This would be lower camel case in llvm style. (But honestly, I'm not sure if this function adds any value).
Okay, I've removed it.


================
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)
----------------
labath wrote:
> Huh?
I did get this compile error before:


```
compile error: no memcpy in std ns

/home/kkleine/llvm/llvm/include/llvm/Support/MathExtras.h:271:8: error: no member named 'memcpy' in namespace 'std'; did you mean 'wmemcpy'?

std::memcpy(in, &Val, sizeof(Val));
~~~~~^~~~~~
     wmemcpy
```

So I "fixed" it along the way. And according to http://www.cplusplus.com/reference/cstring/memcpy/?kw=memcpy, there's no `std::memcpy`


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:201
+  }
   if (!Doc.DynamicSymbols.empty())
     ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
----------------
labath wrote:
> 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.
Okay, I will strip it out of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791