[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

[Pavel Labath via Phabricator via lldb-commits]

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