[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

Thu Feb 23 12:17:57 PST 2017

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

In general code around crashes, we don't want to introduce any crashes in LLDB (no llvm_unreachable, asserts ok if code still functions without them). See inlined comments.

================
Comment at: lldb/include/lldb/Core/DataBufferHeap.h:28
 /// fault new data pages in. Large amounts of data that comes from files
-/// should probably use the DataBufferMemoryMap class.
+/// should probably use llvm::MemoryBuffer::getFile(), which can
+/// intelligently determine when memory mapping is optimal.
----------------
Should this suggest "DataBufferLLVM" instead of directly using "llvm::MemoryBuffer::getFile()"?

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:22
+  DataBufferLLVM(std::unique_ptr<llvm::MemoryBuffer> Buffer)
+      : Buffer(std::move(Buffer)) {}
+
----------------
Someone can construct this with an empty unique pointer. We will then crash if someone calls GetBytes().

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:24-25
+
+  static std::shared_ptr<DataBufferLLVM>
+  CreateFromPath(llvm::StringRef Path, uint32_t Size, uint32_t Offset) {
+    // If the file resides non-locally, pass the volatile flag so that we don't
----------------
If you move this to DataBufferLLVM.cpp you can avoid including  "llvm/Support/MemoryBuffer.h" and just include "ClangForward.h" and make sure "llvm::MemoryBuffer is forward declared. If you do, you will need to declare the destructor and have the implementation in the .cpp file as well.

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:42
+
+  uint8_t *GetBytes() override {
+    llvm_unreachable("Not implemented!");
----------------
We could also split this into two calls:

```
uint8_t *GetWriteableBytes()
{
  if (Buffer->isWriteable())
    return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
  else
    return nullptr;
}

const uint8_t *GetReadableBytes() const
{
    return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
}
```

This would then allow clients to not get the non const version by the sheer fact that their DataBufferLLVM is not const.

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:43
+  uint8_t *GetBytes() override {
+    llvm_unreachable("Not implemented!");
+    return nullptr;
----------------
zturner wrote:
> labath wrote:
> > This makes pretty much everything fail. Most of the code base has a reference to a non-const DataBuffer, which then just segfaults after calling this. I think you'll have to return a const_cast of the buffer here for now.
> That's too bad, although I guess this problem will go away in the future if I can replace `DataBuffer` with either `llvm::MemoryBuffer` or `llvm::BinaryStream`.
So one key benefit of having people share used of a DataBufferSP is that they can all reference the same data and the original buffer is ref counted by that shared pointer. You might do:

```
DataBufferSP data_sp = (load data for entire file);
DataExtractor extractor(data_sp, ....); 
// Above extractor now has shared reference to data_sp
DataExtractor extractor2(extractor, 0x1000, 0x2000);
// Above extractor2 now has a shared reference to data_sp as well, but it only points
// at a subset of the data starting at offset 0x1000, and being 0x2000 bytes in size
return extractor2;
```

Note that if we return extractor2, it has a strong reference to the data and will keep the data alive as "data_sp" and "extractor" will be destroyed, the backing data won't. Any future switch will need to keep this in mind and can't be making copies of the data in the process. This happens quite a bit in LLDB as you mmap the entire contents of a universal file that contains N mach-o files, and might hand out a DataExtractor for the big endian PowerPC slice, and one for the little endian x86_64 slice. This is the primary reason for DataExtractor being able to have a shared reference to the backing data. Keep that in mind when trying to replace with something else.

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:43
+  uint8_t *GetBytes() override {
+    llvm_unreachable("Not implemented!");
+    return nullptr;
----------------
clayborg wrote:
> zturner wrote:
> > labath wrote:
> > > This makes pretty much everything fail. Most of the code base has a reference to a non-const DataBuffer, which then just segfaults after calling this. I think you'll have to return a const_cast of the buffer here for now.
> > That's too bad, although I guess this problem will go away in the future if I can replace `DataBuffer` with either `llvm::MemoryBuffer` or `llvm::BinaryStream`.
> So one key benefit of having people share used of a DataBufferSP is that they can all reference the same data and the original buffer is ref counted by that shared pointer. You might do:
> 
> ```
> DataBufferSP data_sp = (load data for entire file);
> DataExtractor extractor(data_sp, ....); 
> // Above extractor now has shared reference to data_sp
> DataExtractor extractor2(extractor, 0x1000, 0x2000);
> // Above extractor2 now has a shared reference to data_sp as well, but it only points
> // at a subset of the data starting at offset 0x1000, and being 0x2000 bytes in size
> return extractor2;
> ```
> 
> Note that if we return extractor2, it has a strong reference to the data and will keep the data alive as "data_sp" and "extractor" will be destroyed, the backing data won't. Any future switch will need to keep this in mind and can't be making copies of the data in the process. This happens quite a bit in LLDB as you mmap the entire contents of a universal file that contains N mach-o files, and might hand out a DataExtractor for the big endian PowerPC slice, and one for the little endian x86_64 slice. This is the primary reason for DataExtractor being able to have a shared reference to the backing data. Keep that in mind when trying to replace with something else.
> 
> 
Also please don't use llvm_unreachable since that can crash the debugger. You can put in an assert, but the code must function without the assert without crashing.

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:48
+  const uint8_t *GetBytes() const override {
+    return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
+  }
----------------
This will crash if someone constructs with empty unique_ptr. Please don't put an assert or llvm_unreachable in either. Please test the pointer. Move to .cpp if you end up moving CreateFromPath and not including  "llvm/Support/MemoryBuffer.h"

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:52
+  lldb::offset_t GetByteSize() const override {
+    return Buffer->getBufferSize();
+  }
----------------
Test Buffer before using it and move to .cpp if you end up moving CreateFromPath and end up not including  "llvm/Support/MemoryBuffer.h"

================
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:61
+#endif
\ No newline at end of file

----------------
Add newline

================
Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:40

+#include "llvm/Support/MemoryBuffer.h"
+
----------------
Remove, This is being included by DataBufferLLVM.h and no local uses of MemoryBuffer are in this file.

================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:36
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/MipsABIFlags.h"
----------------
Remove, This is being included by DataBufferLLVM.h and no local uses of MemoryBuffer are in this file.

================
Comment at: lldb/unittests/Process/minidump/MinidumpParserTest.cpp:30
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
----------------
Why is this needed?

https://reviews.llvm.org/D30054