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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
