<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 23, 2013 at 9:52 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">>>Unless you want to test unbuffered files too, it might be better to do<br>
>>something like<br>
>>{<br>
>> raw_fd_ostream OF(TestFD, true, true);<br>
>>....<br>
>>}<br>
>>OwningBuffer Buf;<br>
><br>
> I don't mind about buffering - really just wanted the results to be seen in<br>
> the file immediately without closing it. The new patch explicitly separates<br>
> the two cases: (1) file is still open and (2) file is close and then<br>
> reopened for reading.<br>
<br>
</div>Yes, with the second patch we are good. One nit is that you might want<br>
to open with buffering when it will be closed, to make sure buffers<br>
are flushed.<br>
<div class="im"><br></div></blockquote><div><br></div><div>OK, will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
>>You probably want to pass the buff size, not the filesize down to<br>
>>MemoryBuffer::getOpenFile.<br>
><br>
> You are probably right; I was following the usage of getOpenFile within<br>
> gold-plugin.cpp, which deals with "files within files" and also passes the<br>
> size of the sub-file into FileSize. Now looking more closely on that<br>
> invocation, something doesn't make sense: When MapSize = -1, getOpenFile<br>
> sets MapSize = FileSize. But then, shouldUseMmap asserts:<br>
<br>
</div>Oops. That is probably my bug. The FileSize being passed down should<br>
be what stat would return. Maybe we should add an assert?<br></blockquote><div><br></div><div>That would be a good idea. Anything that clarifies the intention of the API.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div class="im"><br>
>   size_t End = Offset + MapSize;<br>
>   assert(End <= FileSize);<br>
><br>
> Meaning that if a positive offset is passed, and MapSize = 1 this assertion<br>
> would fire (unless shouldUseMmap bails out for small sizes).<br>
<br>
</div>You mean MapSize == -1, right? But yes, shouldUseMmap should return<br>
false for small map sizes, even if they are in large files.<br></blockquote><div><br></div><div>Yes, -1, sorry about the typo. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


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

<div><br></div><div>Eli</div><div> </div></div></div></div>