[PATCH] run the unit test for MemoryBuffer::getOpenFile in two different modes

[Eli Bendersky]

On Tue, Jul 23, 2013 at 9:52 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> >>Unless you want to test unbuffered files too, it might be better to do
> >>something like
> >>{
> >> raw_fd_ostream OF(TestFD, true, true);
> >>....
> >>}
> >>OwningBuffer Buf;
> >
> > I don't mind about buffering - really just wanted the results to be seen
> in
> > the file immediately without closing it. The new patch explicitly
> separates
> > the two cases: (1) file is still open and (2) file is close and then
> > reopened for reading.
>
> Yes, with the second patch we are good. One nit is that you might want
> to open with buffering when it will be closed, to make sure buffers
> are flushed.
>
>
OK, will do.


> >>You probably want to pass the buff size, not the filesize down to
> >>MemoryBuffer::getOpenFile.
> >
> > You are probably right; I was following the usage of getOpenFile within
> > gold-plugin.cpp, which deals with "files within files" and also passes
> the
> > size of the sub-file into FileSize. Now looking more closely on that
> > invocation, something doesn't make sense: When MapSize = -1, getOpenFile
> > sets MapSize = FileSize. But then, shouldUseMmap asserts:
>
> Oops. That is probably my bug. The FileSize being passed down should
> be what stat would return. Maybe we should add an assert?
>

That would be a good idea. Anything that clarifies the intention of the API.


>
> >   size_t End = Offset + MapSize;
> >   assert(End <= FileSize);
> >
> > Meaning that if a positive offset is passed, and MapSize = 1 this
> assertion
> > would fire (unless shouldUseMmap bails out for small sizes).
>
> You mean MapSize == -1, right? But yes, shouldUseMmap should return
> false for small map sizes, even if they are in large files.
>

Yes, -1, sorry about the typo.


>
> > So is gold-plugin.cpp using the API incorrectly? Note that actually using
> > this Offset is uncommon, ISTM only gold/lto related code uses it.
> >
> > [in any case it would probably be beneficial to explain the API in more
> > detail in a comment - I can do that with my next commit there]
>
> Do you want me to try to add the assert?
>

Sure. But one question remains - is gold-plugin using the API of
getOpenFile incorrectly by passing a positive Offset, and at the same time
passing the buffer size into FileSize rather than MapSize?

Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130723/970b5fa7/attachment.html>