It looks like that in this case (and possibly many others) a wrapper
class is not necessary. If we just want to log the inputs and outputs
of mmap(), we just need to wrap the factory function (as we have done
here) and let the users access the llvm class directly.
On 27 February 2017 at 20:05,
Having library functions that don't return good errors seems like such an
obvious failing that it shouldn't be hard to motivate fixing that. Then our
logging can go in the wrapper classes, using those errors. That seems like a
pattern that solves the "don't duplicate code" problem and the
Oh that wasn't in response to the comment about wrapper classes, that was
just a general comment about the fact that we lose logging by moving to
LLVM's implementation at all. If we have our own implementation, we could
in theory log at various stages of the mmaping process, but by moving to a
I worry about stripping out the wrappers, because there are some differences in
how lldb operates from llvm that I don't think we want to push down into llvm -
in this case I'm thinking particularly about logging. DataBufferMemoryMap did
a bunch of logging, which presumably would get lost if
I didn't refer to mmaping in the name because LLVM's MemoryBuffer is not
necessarily mmap'ed. It might be mmap'ed and it might not be. Depends on
various factors such as whether you specify the IsVolatile flag, how big
the file is, and maybe a few other things.
After this change we have
This is kind of after the fact, but why didn't we reuse DataBufferMemoryMap for
the Memory Map data buffer that now happens to be backed by an LLVM
implementation? DataBufferLLVM doesn't really tell anybody what the thing does
w/o looking up the implementation.
Jim
> On Feb 27, 2017, at
I was thinking of a simple test like "call get on an existing file and
make sure it returns something reasonable" and "call get on a
non-existing file and make sure it returns null". This is a very thin
wrapper over over the llvm code, so I don't insist on it though...
On 24 February 2017 at
This revision was automatically updated to reflect the committed changes.
Closed by commit rL296159: Delete DataBufferMemoryMap. (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D30054?vs=89547=89701#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30054
Files:
labath added a comment.
I am not sure if this is a voting situation, but I agree with what Zachary said
above.
Since we're already speaking about tests, it looks like the new DataBufferLLVM
class could use a unit test or two, just so we get in the habit of writing
those.
zturner added a comment.
The main concern I have with adding null checks is that I think it actually
*increases* your risk of crashing. All of a sudden you've introduced an
entirely new branch / code-path that is completely untested. Worse, it only
occurs in a situation where you've
clayborg added a comment.
I would still vote to check Buffer for NULL. GetByteSize() and GetBytes() are
usually accessed one time so there won't be a performance issue. If anyone
wanted to actually use a DataBufferLLVM as a member variable, they would need
to use a std::unique_ptr to one with
zturner updated this revision to Diff 89547.
zturner added a comment.
Updated with suggestions from clayborg@. It seems wasteful to me check the
pointer on every single call to read when we can check it once on creation. So
I've added an assert in the constructor and documented with doxygen
zturner updated this revision to Diff 89542.
https://reviews.llvm.org/D30054
Files:
lldb/include/lldb/Core/DataBufferHeap.h
lldb/include/lldb/Core/DataBufferLLVM.h
lldb/include/lldb/Core/DataBufferMemoryMap.h
lldb/include/lldb/Host/FileSpec.h
lldb/source/Core/CMakeLists.txt
zturner added inline comments.
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:43
+ uint8_t *GetBytes() override {
+llvm_unreachable("Not implemented!");
+return nullptr;
labath wrote:
> This makes pretty much everything fail. Most of the code base
labath added a comment.
I've tried this out on linux. I got it working only after making the following
modifications:
Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:15
+#include "lldb/Host/FileSpec.h"
+#include "llvm/Support/MemoryBuffer.h"
+
add
zturner updated this revision to Diff 89453.
zturner added a comment.
Updated with suggestions from labath@
https://reviews.llvm.org/D30054
Files:
lldb/include/lldb/Core/DataBufferHeap.h
lldb/include/lldb/Core/DataBufferLLVM.h
lldb/include/lldb/Core/DataBufferMemoryMap.h
labath added a comment.
I think you are still waiting to get the llvm changes sorted out, but this side
of it looks fine to me (modulo a couple of nits).
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:414
+
+data_sp =
zturner created this revision.
Herald added a subscriber: mgorny.
This depends on https://reviews.llvm.org/D30010 going in first, but assuming
that's successful, this patch updates LLDB to use LLVM's memory mapping instead
of `DataBufferMemoryMap`. Since this also makes `DataBufferMemoryMap`
18 matches
Mail list logo
