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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:337
+find_package(LibLZMA)
+cmake_dependent_option(LLDB_ENABLE_LZMA "Support LZMA compression" ON "LIBLZMA_FOUND" OFF)
+set(LLDB_LIBLZMA_LIBRARIES)
----------------
This looks like a really useful macro. I'll be sure to remember it.


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:338
+cmake_dependent_option(LLDB_ENABLE_LZMA "Support LZMA compression" ON "LIBLZMA_FOUND" OFF)
+set(LLDB_LIBLZMA_LIBRARIES)
+if (LLDB_ENABLE_LZMA)
----------------
It doesn't look like this is used anywhere.


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:341
+  if (LIBLZMA_FOUND)
+	add_definitions(-DLLDB_ENABLE_LZMA)
+    include_directories(${LIBLZMA_INCLUDE_DIRS})
----------------
Please put this into `Config.h.cmake`. I know a lot of code in this file does not do that, but that's because we didn't have a Config.h back then.


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:344
+  else()
+    message(FATAL_ERROR "LZMA devel package is required when LLDB_ENABLE_LZMA is On.")
+  endif()
----------------
Is this code actually reachable? My understanding is that `LLDB_ENABLE_LZMA` should be automatically false if LIBLZMA_FOUND is not true...


================
Comment at: lldb/source/Host/common/LZMA.cpp:81-82
+        llvm::inconvertibleErrorCode(),
+        "size of xz-compressed blob ({0} bytes) is smaller than the "
+        "LZMA_STREAM_HEADER_SIZE ({1} bytes)",
+        compressedBufferSize, LZMA_STREAM_HEADER_SIZE);
----------------
printf format string here.


================
Comment at: lldb/source/Host/common/LZMA.cpp:91
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "lzma_stream_footer_decode()={%s}",
+                                   convertLZMACodeToString(xzerr).data());
----------------
This is a valid printf string, but I am not sure if you intended it to come out like `{lzma error: LZMA_MEMLIMIT_ERROR}`


================
Comment at: lldb/source/Host/common/LZMA.cpp:92
+                                   "lzma_stream_footer_decode()={%s}",
+                                   convertLZMACodeToString(xzerr).data());
+  }
----------------
Calling `.data()` on a StringRef and expecting to get a c string is ill-advised. `convertLZMACodeToString` is a local static function, you could just make it return a `const char *`...


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1901
+  ArchSpec spec = m_gnuDebugDataObjectFile->GetArchitecture();
+  if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec)) {
+    return m_gnuDebugDataObjectFile;
----------------
kwk wrote:
> labath wrote:
> > Are you sure you want to do that? Presumably, the architecture of the module was already set by the containing object file, and that file probably has a more correct understanding of what the right architecture is...
> Correct, the containing object file has a better understanding of things. Are you suggesting to do this then?
> 
> ```
> ArchSpec spec = GetArchitecture();
>   if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec))
>     return m_gnuDebugDataObjectFile;
> ```
No, I think you should just delete this bit of code completely (as the containing object file will call `SetModulesArchitecture` on its own).


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h:157-164
+  /// Takes the .gnu_debugdata and returns the decompressed object file that is
+  /// stored within that section.
+  ///
+  /// \returns either the decompressed object file stored within the
+  /// .gnu_debugdata section or \c nullptr if an error occured or if there's no
+  /// section with that name.
+  std::shared_ptr<ObjectFileELF> GetGnuDebugDataObjectFile();
----------------
This doesn't need to be public now, right?


================
Comment at: lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp:70
 
+  std::unique_ptr<SymbolVendorELF> symbol_vendor(
+      new SymbolVendorELF(module_sp));
----------------
Using a unique_ptr is not a bad idea, but as right now this patch does not need to touch this file, it shouldn't be done as a part of this patch. Feel free to submit the unique_ptr thingy as a separate patch. No need to go through the review process...


================
Comment at: lldb/test/CMakeLists.txt:119-121
+if (LLVM_ENABLE_LZMA)
+  add_dependencies(lldb-test-deps llvm-nm llvm-strip)
+endif()
----------------
I'd just make this unconditional, and add the tools to the big lldb-test-deps block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791