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 lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits