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