[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 accepted this revision.
augusto2112 added a comment.

I have one nit that I'll leave up to you whether it should be addressed now or not.



================
Comment at: lldb/source/Host/common/File.cpp:545
       num_bytes = bytes_read;
-  } else if (StreamIsValid()) {
+  } else if (ValueGuard file_lock = StreamIsValid()) {
     bytes_read = ::fread(buf, 1, num_bytes, m_stream);
----------------
I believe `descriptor_guard` will still be in scope in the else branch (weird scoping rules), but 


================
Comment at: lldb/source/Host/common/File.cpp:609
       num_bytes = bytes_written;
-  } else if (StreamIsValid()) {
+  } else if (ValueGuard stream_guard = StreamIsValid()) {
     bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
----------------
One thing that's not super obvious for people reading this is that `descriptor_guard` is still in scope in the else branch (due to C++'s weird scoping rules when it comes to declaring variables inside `if` statements), and will keep the descriptor mutex locked which is probably not what you intended. I don't think this affects the correctness of this implementation at the moment, but could be dangerous with further changes on top of this one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157347/new/

https://reviews.llvm.org/D157347