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, Jim
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 "lld
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
lib
> On Feb 27, 2017, at 11:49 AM, Zachary Turner wrote:
>
> There may be some cases where we're no longer mmaping where we used to, but
> looking at LLVM's implementation, that would only happen if the file is
> fairly small, and there's a comment in LLVM explaining why they don't mmap
> small
There may be some cases where we're no longer mmaping where we used to, but
looking at LLVM's implementation, that would only happen if the file is
fairly small, and there's a comment in LLVM explaining why they don't mmap
small files. So I think that's actually an improvement for LLDB (although
I
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 yo
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 DataBuff
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 2:56
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 15:18
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&id=89701#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30054
Files:
I am going to move the constructor to private for now, this should
eliminate any disagreement about whether to add null checks, as it is now
literally impossible to construct one with a null pointer. For now this
doesn't matter since nobody is even using the MemoryBuffer constructor
except from in
I left out unit tests since we'd essentially be duplicating the unit tests
of MemoryBuffer, and because it involves the file system (also this is
temporary code until DataBuffer stuff goes away). Lmk if you disagree
though
On Fri, Feb 24, 2017 at 2:53 AM Pavel Labath via Phabricator <
revi...@revie
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.
https://reviews.llvm
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 explicitl
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 t
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 co
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.
==
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
lldb/sourc
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 h
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 `#includ
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
lldb/include/ll
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 = std::make_shared(std::move(*Buffer));
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`
o
23 matches
Mail list logo
