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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

@labath I did go over a lot of your comments. Thank you for helping me in becoming in a better contributor. I'm still not done with the python/make -> lit test transition but the rest is mostly covered.



================
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)
----------------
labath wrote:
> This looks like a really useful macro. I'll be sure to remember it.
Make sure to include `include(CMakeDependentOption)`


================
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)
----------------
labath wrote:
> It doesn't look like this is used anywhere.
Right, that was a left-over. Removed in 7dc045e67bb. 


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:341
+  if (LIBLZMA_FOUND)
+	add_definitions(-DLLDB_ENABLE_LZMA)
+    include_directories(${LIBLZMA_INCLUDE_DIRS})
----------------
labath wrote:
> labath wrote:
> > 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.
> (Among other things, this helps with incremental rebuilds, as only the files which include `Config.h` need to be recompiled)
Addressed in a6277c07451


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

`LLDB_ENABLE_LZMA` is `ON` if `LIBLZMA_FOUND` is `ON`.

Until now I though that can always go ahead and manually toggle on the option `LLDB_ENABLE_LZMA` by hand. And if you do this when LZMA is not available, then you get the fatal error. But as I understand [1] after reading it more than once, I think you're right: "This macro presents an option to the user only if a set of other conditions are true."

Let me verify this with a small example... yes, you're right. Even if you specify `cmake . -DLLVM_ENABLE_LZMA` it will not enable the option. Awesome! Fixed in 156f0af8c29980139ff1a2a8e51a768050564898

[1]: https://cmake.org/cmake/help/v3.0/module/CMakeDependentOption.html 


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1901
+  ArchSpec spec = m_gnuDebugDataObjectFile->GetArchitecture();
+  if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec)) {
+    return m_gnuDebugDataObjectFile;
----------------
labath wrote:
> 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).
Let me try if it works how you suggested it. No, it doesn't work as no breakpoint from the embedded .gnu_debugdata section will be hit anymore. This section was inspired by `ObjectFileELF::CreateInstance` as you suggested to look at once.


================
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();
----------------
labath wrote:
> This doesn't need to be public now, right?
Right, now that it is only called from CreateSections, it can be made private again. Fixed in bb1f3f0b5bd.


================
Comment at: lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp:70
 
+  std::unique_ptr<SymbolVendorELF> symbol_vendor(
+      new SymbolVendorELF(module_sp));
----------------
labath wrote:
> 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...
It wasn't an idea but a necessity back when the whole minidebuginfo stuff was here. It still is a left-over that I have remove this change.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791