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

[Pavel Labath via lldb-commits]

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 Ingham  wrote:
> 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 "lldb needs 
> more logging" problems nicely.
>
> Jim
>
>> On Feb 27, 2017, at 12:00 PM, Zachary Turner  wrote:
>>
>> 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 
>> library implementation we can only log before or after.
>>
>> On Mon, Feb 27, 2017 at 11:55 AM Jim Ingham  wrote:
>>
>> > 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 files.  So I think that's actually an improvement for LLDB 
>> > (although I doubt we are frequently mapping very small files, so it might 
>> > be a non issue).
>> >
>> > You are correct that we lose some logging this way, but in exchange we get 
>> > better tested code.  The code in question that we lose though is basically 
>> > just the call to mmap and the setup to prepare that call.  As long as we 
>> > have the return code from mmap, we should be able to log the error just as 
>> > gracefully.  Granted, the LLVM code doesn't return an error code right 
>> > now, but it seems like a worthy improvement.
>> >
>> > I'm not sure where the right balance is between facilitating post-mortem 
>> > diagnostics and reducing the amount of problems that happen in the first 
>> > place, but I feel kind of dirty duplicating code just so that we can add 
>> > logging.  Maybe there's a case to be made for adding logging hooks into 
>> > LLVM.
>>
>> Why do wrapper classes mean you get significantly less well tested code?  If 
>> they are just to support logging, you really only need to know that the 
>> values are passed correctly, which is easy to test, and then you could test 
>> that the logging is correct, though we don't currently do logging tests for 
>> the most part.
>>
>> Jim
>>
>>
>> >
>> >
>> > On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham  wrote:
>> > 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 you just went directly to the llvm classes.  Most of the 
>> > tools that build out of the llvm toolset are one-pass tools, and 
>> > reproducing problems with their operation is generally not terribly hard.  
>> > So having a rich logging feature is not all that necessary, and the burden 
>> > of adding it to the code not worth the benefit.  But lldb is much more 
>> > state dependent, and is exposed to a wider variety of the vagaries of 
>> > system behavior, and being both an interactive tool and the API's for GUI 
>> > debuggers, has a wider and more unpredictable array of of use cases.  
>> > Having good logging has saved my day many and many's the time over the 
>> > years, and I don't want to lose that.
>> >
>> > So while I don't have any objection to changing the backing 
>> > implementation, I still see value in the wrapper classes.
>> >
>> > You say LLVM memory buffers can be heap or mmap, but presumably when used 
>> > by what used to be DataBufferMemoryMap they are using the memory map 
>> > version, right?  Still seems useful to have the class name say what the 
>> > thing is.
>> >
>> > Jim
>> >
>> > > On Feb 27, 2017, at 10:40 AM, Zachary Turner  wrote:
>> > >
>> > > 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 DataBufferLLVM and DataBufferHeap.  But it 
>> > > turns out an LLVM MemoryBuffer can also be backed by the heap, which now 
>> > > makes DataBufferHeap redundant as well.  So I think longer term we might 
>> > > be able to get rid of all of LLDB's DataBuffer stuff entirely, and 
>> > > everything w

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

[Jim Ingham via lldb-commits]

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 "lldb needs more 
logging" problems nicely.

Jim

> On Feb 27, 2017, at 12:00 PM, Zachary Turner  wrote:
> 
> 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 
> library implementation we can only log before or after.
> 
> On Mon, Feb 27, 2017 at 11:55 AM Jim Ingham  wrote:
> 
> > 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 files.  So I think that's actually an improvement for LLDB (although 
> > I doubt we are frequently mapping very small files, so it might be a non 
> > issue).
> >
> > You are correct that we lose some logging this way, but in exchange we get 
> > better tested code.  The code in question that we lose though is basically 
> > just the call to mmap and the setup to prepare that call.  As long as we 
> > have the return code from mmap, we should be able to log the error just as 
> > gracefully.  Granted, the LLVM code doesn't return an error code right now, 
> > but it seems like a worthy improvement.
> >
> > I'm not sure where the right balance is between facilitating post-mortem 
> > diagnostics and reducing the amount of problems that happen in the first 
> > place, but I feel kind of dirty duplicating code just so that we can add 
> > logging.  Maybe there's a case to be made for adding logging hooks into 
> > LLVM.
> 
> Why do wrapper classes mean you get significantly less well tested code?  If 
> they are just to support logging, you really only need to know that the 
> values are passed correctly, which is easy to test, and then you could test 
> that the logging is correct, though we don't currently do logging tests for 
> the most part.
> 
> Jim
> 
> 
> >
> >
> > On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham  wrote:
> > 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 you just went directly to the llvm classes.  Most of the tools that 
> > build out of the llvm toolset are one-pass tools, and reproducing problems 
> > with their operation is generally not terribly hard.  So having a rich 
> > logging feature is not all that necessary, and the burden of adding it to 
> > the code not worth the benefit.  But lldb is much more state dependent, and 
> > is exposed to a wider variety of the vagaries of system behavior, and being 
> > both an interactive tool and the API's for GUI debuggers, has a wider and 
> > more unpredictable array of of use cases.  Having good logging has saved my 
> > day many and many's the time over the years, and I don't want to lose that.
> >
> > So while I don't have any objection to changing the backing implementation, 
> > I still see value in the wrapper classes.
> >
> > You say LLVM memory buffers can be heap or mmap, but presumably when used 
> > by what used to be DataBufferMemoryMap they are using the memory map 
> > version, right?  Still seems useful to have the class name say what the 
> > thing is.
> >
> > Jim
> >
> > > On Feb 27, 2017, at 10:40 AM, Zachary Turner  wrote:
> > >
> > > 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 DataBufferLLVM and DataBufferHeap.  But it 
> > > turns out an LLVM MemoryBuffer can also be backed by the heap, which now 
> > > makes DataBufferHeap redundant as well.  So I think longer term we might 
> > > be able to get rid of all of LLDB's DataBuffer stuff entirely, and 
> > > everything will just use llvm::MemoryBuffer directly.
> > >
> > > What do you think?
> > >
> > > On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:
> > > 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 t

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

[Zachary Turner via lldb-commits]

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
library implementation we can only log before or after.

On Mon, Feb 27, 2017 at 11:55 AM Jim Ingham  wrote:

>
> > 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 files.  So I think that's actually an improvement for LLDB (although
> I doubt we are frequently mapping very small files, so it might be a non
> issue).
> >
> > You are correct that we lose some logging this way, but in exchange we
> get better tested code.  The code in question that we lose though is
> basically just the call to mmap and the setup to prepare that call.  As
> long as we have the return code from mmap, we should be able to log the
> error just as gracefully.  Granted, the LLVM code doesn't return an error
> code right now, but it seems like a worthy improvement.
> >
> > I'm not sure where the right balance is between facilitating post-mortem
> diagnostics and reducing the amount of problems that happen in the first
> place, but I feel kind of dirty duplicating code just so that we can add
> logging.  Maybe there's a case to be made for adding logging hooks into
> LLVM.
>
> Why do wrapper classes mean you get significantly less well tested code?
> If they are just to support logging, you really only need to know that the
> values are passed correctly, which is easy to test, and then you could test
> that the logging is correct, though we don't currently do logging tests for
> the most part.
>
> Jim
>
>
> >
> >
> > On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham  wrote:
> > 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 you just went directly to the llvm classes.  Most of the
> tools that build out of the llvm toolset are one-pass tools, and
> reproducing problems with their operation is generally not terribly hard.
> So having a rich logging feature is not all that necessary, and the burden
> of adding it to the code not worth the benefit.  But lldb is much more
> state dependent, and is exposed to a wider variety of the vagaries of
> system behavior, and being both an interactive tool and the API's for GUI
> debuggers, has a wider and more unpredictable array of of use cases.
> Having good logging has saved my day many and many's the time over the
> years, and I don't want to lose that.
> >
> > So while I don't have any objection to changing the backing
> implementation, I still see value in the wrapper classes.
> >
> > You say LLVM memory buffers can be heap or mmap, but presumably when
> used by what used to be DataBufferMemoryMap they are using the memory map
> version, right?  Still seems useful to have the class name say what the
> thing is.
> >
> > Jim
> >
> > > On Feb 27, 2017, at 10:40 AM, Zachary Turner 
> wrote:
> > >
> > > 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 DataBufferLLVM and DataBufferHeap.  But it
> turns out an LLVM MemoryBuffer can also be backed by the heap, which now
> makes DataBufferHeap redundant as well.  So I think longer term we might be
> able to get rid of all of LLDB's DataBuffer stuff entirely, and everything
> will just use llvm::MemoryBuffer directly.
> > >
> > > What do you think?
> > >
> > > On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:
> > > 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 AM, Pavel Labath via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> > > >
> > > > 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, Zachary Turner 
> wrote:
> > > >> I left out unit tests 

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

[Jim Ingham via lldb-commits]

> 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 files.  So I think that's actually an improvement for LLDB (although I 
> doubt we are frequently mapping very small files, so it might be a non issue).
> 
> You are correct that we lose some logging this way, but in exchange we get 
> better tested code.  The code in question that we lose though is basically 
> just the call to mmap and the setup to prepare that call.  As long as we have 
> the return code from mmap, we should be able to log the error just as 
> gracefully.  Granted, the LLVM code doesn't return an error code right now, 
> but it seems like a worthy improvement.
> 
> I'm not sure where the right balance is between facilitating post-mortem 
> diagnostics and reducing the amount of problems that happen in the first 
> place, but I feel kind of dirty duplicating code just so that we can add 
> logging.  Maybe there's a case to be made for adding logging hooks into LLVM.

Why do wrapper classes mean you get significantly less well tested code?  If 
they are just to support logging, you really only need to know that the values 
are passed correctly, which is easy to test, and then you could test that the 
logging is correct, though we don't currently do logging tests for the most 
part.

Jim


> 
> 
> On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham  wrote:
> 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 you just went directly to the llvm classes.  Most of the tools that build 
> out of the llvm toolset are one-pass tools, and reproducing problems with 
> their operation is generally not terribly hard.  So having a rich logging 
> feature is not all that necessary, and the burden of adding it to the code 
> not worth the benefit.  But lldb is much more state dependent, and is exposed 
> to a wider variety of the vagaries of system behavior, and being both an 
> interactive tool and the API's for GUI debuggers, has a wider and more 
> unpredictable array of of use cases.  Having good logging has saved my day 
> many and many's the time over the years, and I don't want to lose that.
> 
> So while I don't have any objection to changing the backing implementation, I 
> still see value in the wrapper classes.
> 
> You say LLVM memory buffers can be heap or mmap, but presumably when used by 
> what used to be DataBufferMemoryMap they are using the memory map version, 
> right?  Still seems useful to have the class name say what the thing is.
> 
> Jim
> 
> > On Feb 27, 2017, at 10:40 AM, Zachary Turner  wrote:
> >
> > 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 DataBufferLLVM and DataBufferHeap.  But it turns 
> > out an LLVM MemoryBuffer can also be backed by the heap, which now makes 
> > DataBufferHeap redundant as well.  So I think longer term we might be able 
> > to get rid of all of LLDB's DataBuffer stuff entirely, and everything will 
> > just use llvm::MemoryBuffer directly.
> >
> > What do you think?
> >
> > On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:
> > 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 AM, Pavel Labath via lldb-commits 
> > >  wrote:
> > >
> > > 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, Zachary Turner  wrote:
> > >> 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
> > >>  wrote:
> > >>>
> > >>> labath added a comment.
> > >>>
> > >>> I am not sure if this is a voting situation, but I agree with what 
> > >>> Zachary
> > >>> said abov

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

[Zachary Turner via lldb-commits]

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 doubt we are frequently mapping very small files, so it might be a non
issue).

You are correct that we lose some logging this way, but in exchange we get
better tested code.  The code in question that we lose though is basically
just the call to mmap and the setup to prepare that call.  As long as we
have the return code from mmap, we should be able to log the error just as
gracefully.  Granted, the LLVM code doesn't return an error code right now,
but it seems like a worthy improvement.

I'm not sure where the right balance is between facilitating post-mortem
diagnostics and reducing the amount of problems that happen in the first
place, but I feel kind of dirty duplicating code just so that we can add
logging.  Maybe there's a case to be made for adding logging hooks into
LLVM.


On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham  wrote:

> 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 you just went directly to the llvm classes.  Most of the
> tools that build out of the llvm toolset are one-pass tools, and
> reproducing problems with their operation is generally not terribly hard.
> So having a rich logging feature is not all that necessary, and the burden
> of adding it to the code not worth the benefit.  But lldb is much more
> state dependent, and is exposed to a wider variety of the vagaries of
> system behavior, and being both an interactive tool and the API's for GUI
> debuggers, has a wider and more unpredictable array of of use cases.
> Having good logging has saved my day many and many's the time over the
> years, and I don't want to lose that.
>
> So while I don't have any objection to changing the backing
> implementation, I still see value in the wrapper classes.
>
> You say LLVM memory buffers can be heap or mmap, but presumably when used
> by what used to be DataBufferMemoryMap they are using the memory map
> version, right?  Still seems useful to have the class name say what the
> thing is.
>
> Jim
>
> > On Feb 27, 2017, at 10:40 AM, Zachary Turner  wrote:
> >
> > 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 DataBufferLLVM and DataBufferHeap.  But it
> turns out an LLVM MemoryBuffer can also be backed by the heap, which now
> makes DataBufferHeap redundant as well.  So I think longer term we might be
> able to get rid of all of LLDB's DataBuffer stuff entirely, and everything
> will just use llvm::MemoryBuffer directly.
> >
> > What do you think?
> >
> > On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:
> > 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 AM, Pavel Labath via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> > >
> > > 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, Zachary Turner 
> wrote:
> > >> 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
> > >>  wrote:
> > >>>
> > >>> 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.org/D30054
> > >>>
> > >>>
> > >>>
> > >>
> > > ___
> > > lldb-commits mailing list
> > > lldb-commits@lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> >
>

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

[Jim Ingham via lldb-commits]

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 you just went directly 
to the llvm classes.  Most of the tools that build out of the llvm toolset are 
one-pass tools, and reproducing problems with their operation is generally not 
terribly hard.  So having a rich logging feature is not all that necessary, and 
the burden of adding it to the code not worth the benefit.  But lldb is much 
more state dependent, and is exposed to a wider variety of the vagaries of 
system behavior, and being both an interactive tool and the API's for GUI 
debuggers, has a wider and more unpredictable array of of use cases.  Having 
good logging has saved my day many and many's the time over the years, and I 
don't want to lose that.

So while I don't have any objection to changing the backing implementation, I 
still see value in the wrapper classes.

You say LLVM memory buffers can be heap or mmap, but presumably when used by 
what used to be DataBufferMemoryMap they are using the memory map version, 
right?  Still seems useful to have the class name say what the thing is.

Jim

> On Feb 27, 2017, at 10:40 AM, Zachary Turner  wrote:
> 
> 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 DataBufferLLVM and DataBufferHeap.  But it turns 
> out an LLVM MemoryBuffer can also be backed by the heap, which now makes 
> DataBufferHeap redundant as well.  So I think longer term we might be able to 
> get rid of all of LLDB's DataBuffer stuff entirely, and everything will just 
> use llvm::MemoryBuffer directly.
> 
> What do you think?
> 
> On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:
> 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 AM, Pavel Labath via lldb-commits 
> >  wrote:
> >
> > 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, Zachary Turner  wrote:
> >> 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
> >>  wrote:
> >>>
> >>> 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.org/D30054
> >>>
> >>>
> >>>
> >>
> > ___
> > lldb-commits mailing list
> > lldb-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

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 DataBufferLLVM and DataBufferHeap.  But it turns
out an LLVM MemoryBuffer can also be backed by the heap, which now makes
DataBufferHeap redundant as well.  So I think longer term we might be able
to get rid of all of LLDB's DataBuffer stuff entirely, and everything will
just use llvm::MemoryBuffer directly.

What do you think?

On Mon, Feb 27, 2017 at 10:36 AM Jim Ingham  wrote:

> 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 AM, Pavel Labath via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > 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, Zachary Turner  wrote:
> >> 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
> >>  wrote:
> >>>
> >>> 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.org/D30054
> >>>
> >>>
> >>>
> >>
> > ___
> > lldb-commits mailing list
> > lldb-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jim Ingham via lldb-commits]

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 AM, Pavel Labath via lldb-commits 
>  wrote:
> 
> 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, Zachary Turner  wrote:
>> 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
>>  wrote:
>>> 
>>> 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.org/D30054
>>> 
>>> 
>>> 
>> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via lldb-commits]

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, Zachary Turner  wrote:
> 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
>  wrote:
>>
>> 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.org/D30054
>>
>>
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

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:
  lldb/trunk/include/lldb/Core/DataBufferHeap.h
  lldb/trunk/include/lldb/Core/DataBufferLLVM.h
  lldb/trunk/include/lldb/Core/DataBufferMemoryMap.h
  lldb/trunk/include/lldb/Host/FileSpec.h
  lldb/trunk/source/Core/CMakeLists.txt
  lldb/trunk/source/Core/DataBufferLLVM.cpp
  lldb/trunk/source/Core/DataBufferMemoryMap.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  
lldb/trunk/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
  lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -19,13 +19,15 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 // C includes
@@ -49,11 +51,11 @@
   void SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX) {
 llvm::SmallString<128> filename = inputs_folder;
 llvm::sys::path::append(filename, minidump_filename);
-FileSpec minidump_file(filename.c_str(), false);
-lldb::DataBufferSP data_sp(
-minidump_file.MemoryMapFileContents(0, load_size));
+
+auto BufferPtr = DataBufferLLVM::CreateFromPath(filename, load_size, 0);
+
 llvm::Optional optional_parser =
-MinidumpParser::Create(data_sp);
+MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
Index: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -12,7 +12,7 @@
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
-#include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -25,6 +25,7 @@
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
@@ -50,20 +51,25 @@
 
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
-  const size_t header_size = sizeof(MinidumpHeader);
-  lldb::DataBufferSP data_sp(crash_file->MemoryMapFileContents(0, header_size));
-  if (!data_sp)
+  constexpr size_t header_size = sizeof(MinidumpHeader);
+  auto DataPtr =
+  DataBufferLLVM::CreateFromFileSpec(*crash_file, header_size, 0);
+  if (!DataPtr)
 return nullptr;
 
+  assert(DataPtr->GetByteSize() == header_size);
+
   // first, only try to parse the header, beacuse we need to be fast
-  llvm::ArrayRef header_data(data_sp->GetBytes(), header_size);
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  llvm::ArrayRef HeaderBytes = DataPtr->GetData();
+  const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
+  if (header == nullptr)
+return nullptr;
 
-  if (data_sp->GetByteSize() != header_size || header == nullptr)
+  auto AllData = DataBufferLLVM::CreateFromFileSpec(*crash_file, -1, 0);
+  if (!AllData)
 return nullptr;
 
-  lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());
-  auto minidump_parser = MinidumpParser::Create(all_data_sp);
+  auto minidump_parser = MinidumpParser::Create(AllData);
   // check if the parser object is valid
   if (!minidump_parser)
 return nullptr;
Index: lldb/trunk/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
===
--- lldb/trunk/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
+++ lldb/trunk/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
@@ -28,15 +28,17 @@
 #endif
 
 #include "lldb/Core/ArchSpec.h"
-#include "lldb/Core/DataBuffer.h"
+#include "

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

[Zachary Turner via lldb-commits]

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 internally.  We can re-visit this in the future if we have need
for external users to construct one with a MemoryBuffer.

On Fri, Feb 24, 2017 at 7:18 AM Zachary Turner  wrote:

> 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...@reviews.llvm.org> wrote:
>
> 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.org/D30054
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

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...@reviews.llvm.org> wrote:

> 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.org/D30054
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

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.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

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 explicitly told the user not to do 
something, and they accidentally did it anyway.

As a general rule, you just can't protect against that, and you shouldn't try 
to because there are an infinite number of ways it could occur in practice.  
It's the halting problem.  If you tell the user of an API to check a pointer 
for null before using the API, then you can make a ton of assumptions inside 
the API that greatly simplify things and increase test coverage.  Once you 
start propagating this kind of thing up through the API, tons of error checking 
and error handling code (which is all likely untested, and nobody understands 
how it can ever happen or if it's even possible to happen) just goes away 
because there is just no way for those errors to happen anymore.  (In fact, in 
many cases maybe the errors already can't happen, but there is so much 
propagating of null checks through various levels of the API that you can't 
really reason about whether it does or doesn't).

If an API documents that it never returns NULL, for example, then you don't 
need to check the return value for null.  Just like if an API documents that it 
doesn't accept null, then you do need to check the parameter you pass in.

BTW, not sure if this interests you, but C++ people have been working on the 
C++ Core Guidelines, some of which will eventually make it into the standard.  
One of those things is a class called `not_null` (link here 
) which restricts 
a pointer to be non null.  That would solve a lot of problems like this at the 
type system level, and the conversion will be much easier / less painful / less 
churn if we already know which pointers we can make assumptions about.


https://reviews.llvm.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Greg Clayton via Phabricator via lldb-commits]

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 the current designed because it can't be 
default constructed.




Comment at: lldb/source/Core/DataBufferLLVM.cpp:20-21
+: Buffer(std::move(Buffer)) {
+  assert(Buffer != nullptr &&
+ "Cannot construct a DataBufferLLVM with a null buffer");
+}

This doesn't stop things from crashing if for some reason someone does do this 
and asserts are off. The DataBuffer classes are there for keeping the data 
alive. The main class that uses the DataBuffer classes is DataExtractor and 
that grabs the data pointer on time and stores it into the DataExtractor class. 
So I vote for letting this be empty and checking the unique_ptr as GetBytes is 
usually accessed one time.



Comment at: lldb/source/Core/DataBufferLLVM.cpp:52
+lldb::offset_t DataBufferLLVM::GetByteSize() const {
+  return Buffer->getBufferSize();
+}

I would still vote to check Buffer for NULL. As I said above, GetByteSize() and 
GetBytes() are usually accessed one time.



Comment at: lldb/source/Core/DataBufferLLVM.cpp:56
+const uint8_t *DataBufferLLVM::GetBuffer() const {
+  return reinterpret_cast(Buffer->getBufferStart());
+}

I would still vote to check Buffer for NULL. As I said above, GetByteSize() and 
GetBytes() are usually accessed one time.


https://reviews.llvm.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

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 comments 
that the class should not be constructed with a non-null `MemoryBuffer`.  i.e. 
it's the caller's responsibility to ensure the pointer is not null.

Note that when creating one from a path using the supplied static functions, 
this is not an issue since it will never create one with a null pointer anyway.

I would prefer to avoid superfluous null pointer checks, so hopefully this is 
sufficient.


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/source/Core/DataBufferLLVM.cpp
  lldb/source/Core/DataBufferMemoryMap.cpp
  lldb/source/Host/common/FileSpec.cpp
  lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -19,13 +19,15 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 // C includes
@@ -49,11 +51,11 @@
   void SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX) {
 llvm::SmallString<128> filename = inputs_folder;
 llvm::sys::path::append(filename, minidump_filename);
-FileSpec minidump_file(filename.c_str(), false);
-lldb::DataBufferSP data_sp(
-minidump_file.MemoryMapFileContents(0, load_size));
+
+auto BufferPtr = DataBufferLLVM::CreateFromPath(filename, load_size, 0);
+
 llvm::Optional optional_parser =
-MinidumpParser::Create(data_sp);
+MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -12,7 +12,7 @@
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
-#include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -25,6 +25,7 @@
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
@@ -50,20 +51,26 @@
 
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
-  const size_t header_size = sizeof(MinidumpHeader);
-  lldb::DataBufferSP data_sp(crash_file->MemoryMapFileContents(0, header_size));
-  if (!data_sp)
+  constexpr size_t header_size = sizeof(MinidumpHeader);
+  auto DataPtr =
+  DataBufferLLVM::CreateFromFileSpec(*crash_file, header_size, 0);
+  if (!DataPtr)
 return nullptr;
 
+  assert(DataPtr->GetByteSize() == header_size);
+
   // first, only try to parse the header, beacuse we need to be fast
-  llvm::ArrayRef header_data(data_sp->GetBytes(), header_size);
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  llvm::ArrayRef HeaderBytes = DataPtr->GetData();
+  const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
+  if (header == nullptr)
+return nullptr;
 
-  if (data_sp->GetByteSize() != header_size || header == nullptr)
+  auto AllData = llvm::MemoryBuffer::getFile(crash_file->GetPath(), -1, false);
+  if (!AllData)
 return nullptr;
+  auto AllDataPtr = std::make_shared(std::move(*AllData));
 
-  lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());
-  auto minidump_parser = MinidumpParser::Create(all_data_sp);
+  auto minidump_parser = MinidumpParser::Create(AllDataPtr);
   // check if the parser object is valid
   if (!minidump_parser)
 return nullptr;
Index: lldb/source/Plugins/ObjectFile/PECOFF/

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

[Greg Clayton via Phabricator via lldb-commits]

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 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
+  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(Buffer->getBufferStart());
  else
return nullptr;
}

const uint8_t *GetReadableBytes() const
{
return reinterpret_cast(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);

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

[Zachary Turner via Phabricator via lldb-commits]

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/source/Core/DataBufferMemoryMap.cpp
  lldb/source/Host/common/FileSpec.cpp
  lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -19,13 +19,15 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 // C includes
@@ -49,11 +51,11 @@
   void SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX) {
 llvm::SmallString<128> filename = inputs_folder;
 llvm::sys::path::append(filename, minidump_filename);
-FileSpec minidump_file(filename.c_str(), false);
-lldb::DataBufferSP data_sp(
-minidump_file.MemoryMapFileContents(0, load_size));
+
+auto BufferPtr = DataBufferLLVM::CreateFromPath(filename, load_size, 0);
+
 llvm::Optional optional_parser =
-MinidumpParser::Create(data_sp);
+MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -12,7 +12,7 @@
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
-#include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -25,6 +25,7 @@
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
@@ -50,20 +51,26 @@
 
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
-  const size_t header_size = sizeof(MinidumpHeader);
-  lldb::DataBufferSP data_sp(crash_file->MemoryMapFileContents(0, header_size));
-  if (!data_sp)
+  constexpr size_t header_size = sizeof(MinidumpHeader);
+  auto DataPtr =
+  DataBufferLLVM::CreateFromFileSpec(*crash_file, header_size, 0);
+  if (!DataPtr)
 return nullptr;
 
+  assert(DataPtr->GetByteSize() == header_size);
+
   // first, only try to parse the header, beacuse we need to be fast
-  llvm::ArrayRef header_data(data_sp->GetBytes(), header_size);
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  llvm::ArrayRef HeaderBytes = DataPtr->GetData();
+  const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
+  if (header == nullptr)
+return nullptr;
 
-  if (data_sp->GetByteSize() != header_size || header == nullptr)
+  auto AllData = llvm::MemoryBuffer::getFile(crash_file->GetPath(), -1, false);
+  if (!AllData)
 return nullptr;
+  auto AllDataPtr = std::make_shared(std::move(*AllData));
 
-  lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());
-  auto minidump_parser = MinidumpParser::Create(all_data_sp);
+  auto minidump_parser = MinidumpParser::Create(AllDataPtr);
   // check if the parser object is valid
   if (!minidump_parser)
 return nullptr;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -13,8 +13,8 @@
 #include "llvm/Support/COFF.h"
 
 #include "lldb/Core/ArchSpec.h"
-#include "lldb/Core/DataBuffer.h"
 #include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -30,6 +30,8 @@
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "llvm/Support/MemoryBuffer.h"
+
 #define IMAGE_DOS_SIGNATURE 0x5A4D// MZ
 #de

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

[Zachary Turner via Phabricator via lldb-commits]

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 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`.


https://reviews.llvm.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

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 `#include "llvm/Support/FileSystem.h"`



Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:38
+  static std::shared_ptr
+  CreateFromFileSpec(const FileSpec &F, uint32_t Size, uint32_t Offset) {
+return CreateFromPath(F.GetPath(), Size, Offset);

Change this to `uint64_t Size` so that passing -1 will correctly mean 
"auto-detect the file size".



Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:43
+  uint8_t *GetBytes() override {
+llvm_unreachable("Not implemented!");
+return nullptr;

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.


https://reviews.llvm.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

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/lldb/Host/FileSpec.h
  lldb/source/Core/CMakeLists.txt
  lldb/source/Core/DataBufferMemoryMap.cpp
  lldb/source/Host/common/FileSpec.cpp
  lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -19,13 +19,15 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 // C includes
@@ -49,11 +51,11 @@
   void SetUpData(const char *minidump_filename, size_t load_size = SIZE_MAX) {
 llvm::SmallString<128> filename = inputs_folder;
 llvm::sys::path::append(filename, minidump_filename);
-FileSpec minidump_file(filename.c_str(), false);
-lldb::DataBufferSP data_sp(
-minidump_file.MemoryMapFileContents(0, load_size));
+
+auto BufferPtr = DataBufferLLVM::CreateFromPath(filename, load_size, 0);
+
 llvm::Optional optional_parser =
-MinidumpParser::Create(data_sp);
+MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -12,7 +12,7 @@
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
-#include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -25,6 +25,7 @@
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
@@ -50,20 +51,26 @@
 
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
-  const size_t header_size = sizeof(MinidumpHeader);
-  lldb::DataBufferSP data_sp(crash_file->MemoryMapFileContents(0, header_size));
-  if (!data_sp)
+  constexpr size_t header_size = sizeof(MinidumpHeader);
+  auto DataPtr =
+  DataBufferLLVM::CreateFromFileSpec(*crash_file, header_size, 0);
+  if (!DataPtr)
 return nullptr;
 
+  assert(DataPtr->GetByteSize() == header_size);
+
   // first, only try to parse the header, beacuse we need to be fast
-  llvm::ArrayRef header_data(data_sp->GetBytes(), header_size);
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  llvm::ArrayRef HeaderBytes = DataPtr->GetData();
+  const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
+  if (header == nullptr)
+return nullptr;
 
-  if (data_sp->GetByteSize() != header_size || header == nullptr)
+  auto AllData = llvm::MemoryBuffer::getFile(crash_file->GetPath(), -1, false);
+  if (!AllData)
 return nullptr;
+  auto AllDataPtr = std::make_shared(std::move(*AllData));
 
-  lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());
-  auto minidump_parser = MinidumpParser::Create(all_data_sp);
+  auto minidump_parser = MinidumpParser::Create(AllDataPtr);
   // check if the parser object is valid
   if (!minidump_parser)
 return nullptr;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -13,8 +13,8 @@
 #include "llvm/Support/COFF.h"
 
 #include "lldb/Core/ArchSpec.h"
-#include "lldb/Core/DataBuffer.h"
 #include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -30,6 +30,8 @@
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "llvm/Support/Me

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

[Pavel Labath via Phabricator via lldb-commits]

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));
+data_offset = 0;

It looks like this make-a-MemoryBuffer-and-shove-it-in-a-DataBufferLLVM 
functionality could be handled by a utility function, as it is used in a number 
of places.



Comment at: lldb/unittests/Process/minidump/MinidumpParserTest.cpp:54
 llvm::sys::path::append(filename, minidump_filename);
 FileSpec minidump_file(filename.c_str(), false);
+

It looks like this variable is not necessary anymore.


https://reviews.llvm.org/D30054



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

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` 
obsolete, I went ahead and nuked it from orbit while I was at it.


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/source/Core/DataBufferMemoryMap.cpp
  lldb/source/Host/common/FileSpec.cpp
  lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -19,13 +19,15 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 // C includes
@@ -50,10 +52,12 @@
 llvm::SmallString<128> filename = inputs_folder;
 llvm::sys::path::append(filename, minidump_filename);
 FileSpec minidump_file(filename.c_str(), false);
-lldb::DataBufferSP data_sp(
-minidump_file.MemoryMapFileContents(0, load_size));
+
+auto Buffer =
+llvm::MemoryBuffer::getFileSlice(filename.c_str(), load_size, 0);
+auto BufferPtr = std::make_shared(std::move(*Buffer));
 llvm::Optional optional_parser =
-MinidumpParser::Create(data_sp);
+MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -12,7 +12,7 @@
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
-#include "lldb/Core/DataBufferHeap.h"
+#include "lldb/Core/DataBufferLLVM.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -25,6 +25,7 @@
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
@@ -51,19 +52,27 @@
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
   const size_t header_size = sizeof(MinidumpHeader);
-  lldb::DataBufferSP data_sp(crash_file->MemoryMapFileContents(0, header_size));
-  if (!data_sp)
+  auto Header =
+  llvm::MemoryBuffer::getFileSlice(crash_file->GetPath(), header_size, 0);
+  if (!Header)
 return nullptr;
+  assert(*Header);
+
+  auto DataPtr = std::make_shared(std::move(*Header));
+  assert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
-  llvm::ArrayRef header_data(data_sp->GetBytes(), header_size);
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  llvm::ArrayRef HeaderBytes = DataPtr->GetData();
+  const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
+  if (header == nullptr)
+return nullptr;
 
-  if (data_sp->GetByteSize() != header_size || header == nullptr)
+  auto AllData = llvm::MemoryBuffer::getFile(crash_file->GetPath(), -1, false);
+  if (!AllData)
 return nullptr;
+  auto AllDataPtr = std::make_shared(std::move(*AllData));
 
-  lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());
-  auto minidump_parser = MinidumpParser::Create(all_data_sp);
+  auto minidump_parser = MinidumpParser::Create(AllDataPtr);
   // check if the parser object is valid
   if (!minidump_parser)
 return nullptr;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -13,8 +13,8 @@
 #include "llvm/Support/COFF.h"
 
 #include "lldb/Core/ArchSpec.h"
-#include "lldb/Core/DataBuffer.h"
 #include "lldb/Core/DataBuff