[cfe-dev] libclang locking files for write/delete on windows

[Yaron Keren]

Hi Kim,

I think the code you quoted above

    std::error_code EC;
    std::unique_ptr<MemoryBuffer> Result(
        new (NamedBufferAlloc(Filename))
        MemoryBufferMMapFile(RequiresNullTerminator, FD, MapSize, Offset,
EC));
    if (!EC)
      return std::move(Result);

shows EC is an output variable: it is constructed before
MemoryBufferMMapFile() and tested just after. The missing & is probably a
typo.

For a unit test your second approach seems better since it directly tests
MemoryBufferMMapFile: pass MemoryBufferMMapFile invalid FD or MapSize and
test for errors.

Interesting this wen unnoticed. MemoryBufferMMapFile probably gets valid
arguments and never fails.

Yaron


2014-12-11 7:21 GMT+02:00 Kim Gräsman <kim.grasman at gmail.com>:

> Hi Yaron,
>
> On Wed, Dec 10, 2014 at 11:34 PM, Yaron Keren <yaron.keren at gmail.com>
> wrote:
> > Before this bug report gets lost, Kim is correct about the error code EC,
> > it's an output argument for MemoryBufferMMapFile::MemoryBufferMMapFile
> and
> > must be passed by reference, like:
> >
> >
> > Index: lib/Support/MemoryBuffer.cpp
> > ===================================================================
> > --- lib/Support/MemoryBuffer.cpp        (revision 223970)
> > +++ lib/Support/MemoryBuffer.cpp        (working copy)
> > @@ -203,7 +203,7 @@
> >
> >  public:
> >    MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t
> Len,
> > -                       uint64_t Offset, std::error_code EC)
> > +                       uint64_t Offset, std::error_code &EC)
> >        : MFR(FD, false, sys::fs::mapped_file_region::readonly,
> >              getLegalMapSize(Len, Offset), getLegalMapOffset(Offset),
> EC) {
> >      if (!EC) {
> >
> > This passes regression tests.
>
> I started trying to create a unit test in
> llvm/unittests/Support/MemoryBufferTest.cpp yesterday that proves this
> change, but I think I started at the wrong level. I went at trying to
> prove that MemoryBuffer::getOpenFile returned an invalid object on
> error, but that would mean having to force mapping to fail
> (consistently on all platforms).
>
> Maybe it's better to add a test that MemoryBufferMMapFile's
> constructor returns the error code through an out arg? Then It should
> be trivial to test with an invalid FD or something.
>
> Let me know if you have other ideas, I can try and put a patch together
> today.
>
> - Kim
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141211/5ce1dcae/attachment.html>