On Thu, May 21, 2015 at 10:58 AM, Zachary Turner <ztur...@google.com> wrote:

> I feel like it should be the other way around.  If LockFile is taking a
> reference then it is implicitly saying that someone else owns the
> underlying File handle.  But who else cares about the contents of a lock
> file other than the LockFile object itself?
>
> It's not the case in current LockFile usage scenario but I can imagine
following use case - somebody wants to synchronize IO operations on file's
content:

   - With multiple readers and awaiting writers
   - Single writer and awaiting readers

After you locked a file with LockFile you can proceed on IO operations and
be confident that these operations are synchronized - e.g., instead of
zero-size lock file we could have used module's file as synchronization
point and to lock a module file first before pouring data into it..


> Why not just change the LockFile constructor to not take a file descriptor
> but instead take the path?  This is even better because now in
> LockFileWindows you don't even need to use an fd you can just store a
> HANDLE directly, and it gives you more flexibility with how you create the
> file because you can use windows-specific options such as
> FILE_FLAG_DELETE_ON_CLOSE (if you want that, anyway).
>
> If you're concerned about the case where the Open operation fails and you
> want to handle that error, then don't let the constructor take anything,
> but instead you could make a LockFile::Open() method or a static LockFile
> &&LockFile::Create() method.  (by returning r-value reference here you
> could make LockFile object moveable but non-copyable, which makes sense for
> something like a lock file.
>
> On Thu, May 21, 2015 at 10:50 AM Oleksiy Vyalov <ovya...@google.com>
> wrote:
>
>> I'm somewhat concerned about following scenario:
>>
>> File file_obj(...);
>> LockFile lock (file_obj.GetDescriptor ());
>> ..
>>
>> My understanding, that on Windows if LockFile duplicates original handle
>> (i.e. file.GetDescriptor ()) only a duplicated handle will be allowed to
>> call IO functions on this file within locked region, meanwhile file_obj
>> will be blocked on IO.
>> If LockFile just holds a reference to original handle then file_obj can
>> proceed on IO operations in exclusive mode.
>>
>> In order to make it clear that LockFile references File we can make
>>  LockFile to take File& as constructor parameter instead of a raw handle.
>>
>> On Thu, May 21, 2015 at 10:28 AM, Zachary Turner <ztur...@google.com>
>> wrote:
>>
>>> Why does someone other than the LockFile object care about holding a raw
>>> handle? Shouldn't it just hold a reference/pointer to the LockFile?
>>> On Thu, May 21, 2015 at 10:06 AM Oleksiy Vyalov <ovya...@google.com>
>>> wrote:
>>>
>>>> Windows file lock is tied to a file handle - using handle duplication
>>>> effectively applies locking on duplicated handle, not on original, so your
>>>> original handle will be blocked on IO trying to read/write.
>>>> Maybe we can just eliminate CloseHandle call (
>>>> https://github.com/llvm-mirror/lldb/blob/master/source/Host/windows/LockFileWindows.cpp#L50)
>>>> if file will be eventually closed by LockFileWindows caller.
>>>>
>>>> On Thu, May 21, 2015 at 9:58 AM, Zachary Turner <ztur...@google.com>
>>>> wrote:
>>>>
>>>>> I don't experience this crash but looking at the proposed fix it
>>>>> certainly seems like a plausible explanation. Seems to me like something
>>>>> should transfer ownership though rather than duplicating the handle
>>>>>
>>>>> On Thu, May 21, 2015 at 9:52 AM Colin Riley <co...@codeplay.com>
>>>>> wrote:
>>>>>
>>>>>>  Windows 7 64, Debug, latest vs13.
>>>>>>
>>>>>>
>>>>>> On 21/05/2015 17:39, Oleksiy Vyalov wrote:
>>>>>>
>>>>>> Hi Colin,
>>>>>>
>>>>>>  could you give more context about crash - what build configuration
>>>>>> do you use (debug, release,..) and which OS?
>>>>>> I'm running this code on Windows 7 and haven't noticed any failures.
>>>>>>
>>>>>>
>>>>>> On Thu, May 21, 2015 at 8:57 AM, Colin Riley <co...@codeplay.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> Zachary, do you see this on windows at all? Tip for us results in
>>>>>>> crashes when releasing file descriptors without the below fix.
>>>>>>>
>>>>>>> Colin
>>>>>>>
>>>>>>>
>>>>>>> On 19/05/2015 12:52, Aidan Dodds wrote:
>>>>>>>
>>>>>>>  Hi,
>>>>>>>
>>>>>>> We have been seeing a crash on windows when connecting to an android
>>>>>>> target using lldb-server.
>>>>>>> I am not sure if this affects other platforms too.
>>>>>>> I think this was introduced with http://reviews.llvm.org/D9056.
>>>>>>>
>>>>>>> I tracked the crash back to the workings of
>>>>>>> ModuleCache::GetAndPut().
>>>>>>>
>>>>>>> The crash seems to be due to a file descriptor being released twice,
>>>>>>> once by the original "File lock_file" and again by the "LockFile lock" 
>>>>>>> who
>>>>>>> share the same file descriptor.
>>>>>>>
>>>>>>> The file descriptor sharing happens because of this line:
>>>>>>> ModuleCache.cpp @ 164
>>>>>>> LockFile lock (lock_file.GetDescriptor ());
>>>>>>>
>>>>>>> Both destructors attempt to release effectively the same file
>>>>>>> descriptor.  I was able to fix the crash by duplicating the file handle 
>>>>>>> in
>>>>>>> the lock file constructor using _dup().  (patch attached)
>>>>>>> I wasn't sure if this was the right fix however. Has anyone else
>>>>>>> seen this?
>>>>>>> Should "File lock_file" perhaps transfer its file descriptor
>>>>>>> completely rather then share it?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Aidan
>>>>>>>
>>>>>>>
>>>>>>>  _______________________________________________
>>>>>>> lldb-dev mailing 
>>>>>>> listlldb-...@cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> - Colin Riley
>>>>>>> Senior Director,
>>>>>>> Parallel/Graphics Debugger Systems
>>>>>>>
>>>>>>> Codeplay Software Ltd
>>>>>>> 45 York Place, Edinburgh, EH1 3HP
>>>>>>> Tel: 0131 466 0503
>>>>>>> Fax: 0131 557 6600
>>>>>>> Website: http://www.codeplay.com
>>>>>>> Twitter: https://twitter.com/codeplaysoft
>>>>>>>
>>>>>>> This email and any attachments may contain confidential and /or 
>>>>>>> privileged information and is for use by the addressee only. If you are 
>>>>>>> not the intended recipient, please notify Codeplay Software Ltd 
>>>>>>> immediately and delete the message from your computer. You may not copy 
>>>>>>> or forward it,or use or disclose its contents to any other person. Any 
>>>>>>> views or other information in this message which do not relate to our 
>>>>>>> business are not authorized by Codeplay software Ltd, nor does this 
>>>>>>> message form part of any contract unless so stated.
>>>>>>> As internet communications are capable of data corruption Codeplay 
>>>>>>> Software Ltd does not accept any responsibility for any changes made to 
>>>>>>> this message after it was sent. Please note that Codeplay Software Ltd 
>>>>>>> does not accept any liability or responsibility for viruses and it is 
>>>>>>> your responsibility to scan any attachments.
>>>>>>> Company registered in England and Wales, number: 04567874
>>>>>>> Registered office: 81 Linkfield Street, Redhill RH1 6BY
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>  --
>>>>>>  Oleksiy Vyalov | Software Engineer | ovya...@google.com
>>>>>>
>>>>>>
>>>>>> --
>>>>>> - Colin Riley
>>>>>> Senior Director,
>>>>>> Parallel/Graphics Debugger Systems
>>>>>>
>>>>>> Codeplay Software Ltd
>>>>>> 45 York Place, Edinburgh, EH1 3HP
>>>>>> Tel: 0131 466 0503
>>>>>> Fax: 0131 557 6600
>>>>>> Website: http://www.codeplay.com
>>>>>> Twitter: https://twitter.com/codeplaysoft
>>>>>>
>>>>>> This email and any attachments may contain confidential and /or 
>>>>>> privileged information and is for use by the addressee only. If you are 
>>>>>> not the intended recipient, please notify Codeplay Software Ltd 
>>>>>> immediately and delete the message from your computer. You may not copy 
>>>>>> or forward it,or use or disclose its contents to any other person. Any 
>>>>>> views or other information in this message which do not relate to our 
>>>>>> business are not authorized by Codeplay software Ltd, nor does this 
>>>>>> message form part of any contract unless so stated.
>>>>>> As internet communications are capable of data corruption Codeplay 
>>>>>> Software Ltd does not accept any responsibility for any changes made to 
>>>>>> this message after it was sent. Please note that Codeplay Software Ltd 
>>>>>> does not accept any liability or responsibility for viruses and it is 
>>>>>> your responsibility to scan any attachments.
>>>>>> Company registered in England and Wales, number: 04567874
>>>>>> Registered office: 81 Linkfield Street, Redhill RH1 6BY
>>>>>>
>>>>>>
>>>>
>>>>
>>>> --
>>>> Oleksiy Vyalov | Software Engineer | ovya...@google.com
>>>>
>>>
>>
>>
>> --
>> Oleksiy Vyalov | Software Engineer | ovya...@google.com
>>
>


-- 
Oleksiy Vyalov | Software Engineer | ovya...@google.com
_______________________________________________
lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev

Reply via email to