labath added a comment. I think this is pretty good now. The only concern I have is about the placement NativeProcessTestUtils. The way you're including it now looks weird. I think we should put that in a separate place, as per the inline coment.
In D62501#1533483 <https://reviews.llvm.org/D62501#1533483>, @aadsm wrote: > Note about the tests: I only have one test for the GetELFImageInfoAddress > that follows the 32bit version and also makes sure we correctly read the load > bias. Tbh I'm not sure if this is enough but should be easy to add more cases > now. The main advantage of this kind of tests I see is that they make it possible to test what happens if things don't go as expected. You don't have to go overboard on that (you've already done a lot more here than most other code reviews do), but it may be nice to include one test where this lookup fails (e.g. the DT_DEBUG entry is missing, or something). ================ Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:41 #include "lldb/Utility/StringExtractor.h" +#include "llvm/BinaryFormat/ELF.h" #include "llvm/Support/Errno.h" ---------------- I guess this is no longer needed. ================ Comment at: lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp:9 + +#include "../../Host/NativeProcessTestUtils.h" + ---------------- This is the second time we've needed to share code between various unit tests. Let's just try start a convention for putting these into a separate place and see how it develops. I'd say we should put this into `unittests/TestingSupport/Host/NativeProcessTestUtils.h` (so, #includable as "TestingSupport/Host/NativeProcessTestUtils.h"), for the time being. ================ Comment at: lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp:35 + MockProcessELF &process, + std::vector<std::pair<AuxVector::EntryType, uint32_t>> auxv_data) { + auto addr_size = process.GetAddressByteSize(); ---------------- s/vector/ArrayRef ================ Comment at: lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp:45-46 + } + llvm::StringRef stringref((const char *)buffer_sp->GetBytes(), + buffer_sp->GetByteSize()); + return llvm::MemoryBuffer::getMemBufferCopy(stringref, ""); ---------------- you could say `toStringRef(buffer_sp->GetData())` ================ Comment at: lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp:61-64 + llvm::Optional<uint64_t> maybe_phdr_addr = + process.GetAuxValue(AuxVector::AUXV_AT_PHDR); + ASSERT_NE(llvm::None, maybe_phdr_addr); + ASSERT_EQ(phdr_addr, *maybe_phdr_addr); ---------------- `Optional` has overloaded equality operators which "do the right thing", though they are a bit tricky to use with integral types, as they kick in only if the contained type matches exactly. If you changed the type of `phdr_addr` to `uint64_t`, then you could just write `ASSERT_EQ(phdr_addr, process.GetAuxValue(AuxVector::AUXV_AT_PHDR))` here. ================ Comment at: lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp:83-117 + // We're going to set up a fake memory with 2 program headers and 1 entry in + // the dynamic section. + // For simplicity sake they will be consecutive in memory: + // +------------+ + // | PT_PHDR | + // +------------+ + // | PT_DYNAMIC | ---------------- What if we just defined a struct which described the final memory layout, and then gave that as an argument to the FakeMemory object? I'm thinking of something like: ``` struct MemoryContents { Elf32_Phdr phdr_load; Elf32_Phdr phdr_dynamic; Elf32_Dyn dyn_debug; } MC; MC.phdr_load.p_type = PT_DYNAMIC; ... FakeMemory M(&MC, sizeof MC, phdr_addr); // This assumes adding a (const void *, size_t) constructor to the class ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.org/D62501 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
