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
