[Lldb-commits] [PATCH] D83512: [lldb/Module] Allow for the creation of memory-only modules

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath marked an inline comment as done.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:543-544
 
+  if (m_data.ValidOffsetForDataOfSize(offset, size))
+    return DataExtractor(m_data, offset, size);
+
----------------
friss wrote:
> This seems to work in the test, but I have to admit that I'm not 100% sure it's correct given the comment below about wanting to write this buffer.
I think this is fine. I can't find anything wanting to write to this data. Maybe that was the case once but perhaps that code was removed now.


================
Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:27
+
+// This test file intentionally doesn't initialize the FileSystem.
+// Everything in this file should be able to run without requiring
----------------
this is cool


================
Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:30
+// any interaction with the FileSystem class; by keeping it
+// uninitialized, it will assert if anythign try to interact with
+// it.
----------------
s/anythign/anything


================
Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:57
+  auto ExpectedFile = TestFile::fromYaml(R"(
+--- !ELF
+FileHeader:
----------------
Maybe reduce these down? I'm not sure about others, but and elf file with a single `.text` section and no symbols should still perfectly valid.


================
Comment at: lldb/unittests/TestingSupport/TestUtilities.h:39-41
+  TestFile(TestFile &&RHS) : Buffer(std::move(RHS.Buffer)) {
+    RHS.Buffer = llvm::None;
   }
----------------
Since we don't need to do cleanup anymore, we can make `Buffer` a regular `std::string` and rely on the compiler-generated move and copy operations. In fact, we may not even need the `TestFile` class at all as the yaml functions could return a ModuleSpec directly.


================
Comment at: lldb/unittests/TestingSupport/TestUtilities.h:50
 private:
-  TestFile(llvm::StringRef Name, llvm::FileRemover &&Remover)
-      : Name(std::string(Name)) {
-    Remover.releaseFile();
-  }
+  TestFile(std::string &&Buffer) : Buffer(Buffer) {}
+
----------------
you'll need to add `Buffer(std::move(Buffer))` for the `&&` to do anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83512/new/

https://reviews.llvm.org/D83512