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 

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

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

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