bulbazord added inline comments.
================
Comment at: lldb/include/lldb/Host/File.h:425-433
+ ValueGuard DescriptorIsValid() const {
+ m_descriptor_mutex.lock();
+ return ValueGuard(m_descriptor_mutex,
+ File::DescriptorIsValid(m_descriptor));
+ }
+
+ ValueGuard StreamIsValid() const {
----------------
Hmm, this should be okay since URVO is mandatory as of C++17 (meaning this will
not perform a copy and potentially screw up the locking mechanism).
================
Comment at: lldb/source/Host/common/File.cpp:222
+ size_t written = s.size();
+ ;
Write(s.data(), written);
----------------
nit: delete this stray semicolon.
================
Comment at: lldb/source/Host/common/File.cpp:316
Status error;
- if (StreamIsValid()) {
- if (m_own_stream) {
- if (::fclose(m_stream) == EOF)
- error.SetErrorToErrno();
- } else {
- File::OpenOptions rw =
- m_options & (File::eOpenOptionReadOnly | File::eOpenOptionWriteOnly |
- File::eOpenOptionReadWrite);
+ {
----------------
I think you need to grab both mutexes before you can modify either one of them
in `Close`. As an example:
Thread 1 calls Close.
Thread 2 calls GetStream.
Thread 1 grabs the stream_guard and does all the relevant work with it.
Thread 2 grabs the stream_guard and subsequently grabs the descriptor_guard. It
opens the stream again and sets the descriptor to unowned.
Thread 1 resumes, grabs the descriptor_guard, but does nothing with it because
it's unowned. However, it does reset the state of the other member variables.
In this case, the work that Thread 1 did to close the stream was undone by
Thread 2. The File is not closed and is left is a strange half-initialized
state.
Reading through this and thinking about it more, I wonder if `Close` is the
right abstraction for a File that can be touched by multiple threads. If one
thread closes it and the other is still trying to work with it, you could end
up in a weird state no matter what else is going on. Maybe it would make more
sense to have a File have a reference count? Users wouldn't call `File::Close`
directly but just let it go out of scope and allow the destructor to handle
resource management. That might be out of the scope of this patch though.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
