[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added inline comments.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:801-804
+  if (m_filespec_ap.get())
+    return m_filespec_ap->GetSize();
+
+  m_filespec_ap.reset(new FileSpecList());
----------------
I'm afraid of a race condition here. It seems we can occur here in different threads simultaneously (due to the mutex lock at the line 810), but it is not thread-safe to change and read //same// unique_ptr in different threads simultaneously. It is safe to //move// it between threads, but it's not the case. I would guard all actions on unique_ptr with the mutex.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:881-885
+  auto num_modules = ParseDependentModules();
+  auto original_size = files.GetSize();
+
+  for (unsigned i = 0; i < num_modules; ++i)
+    files.AppendIfUnique(m_filespec_ap->GetFileSpecAtIndex(i));
----------------
Also consider the following scenario:

`Thread A`: `GetDependentModules` -> `ParseDependentModules` -> e.g. line 837, `idx` is greater than `0`, but less than `num_entries`;
`Thread B`: `GetDependentModules` (or `DumpDependentModules`) -> `ParseDependentModules` -> `ParseDependentModules` returns `num_modules` less than actual;
`Thread A`: continues to write into `m_filespec_ap`;
`Thread B`: reads `m_filespec_ap` which is modified at the time by `Thread A`.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1100-1104
+  auto num_modules = ParseDependentModules();
+  if (num_modules > 0) {
+    s->PutCString("Dependent Modules\n");
+    for (unsigned i = 0; i < num_modules; ++i) {
+      auto spec = m_filespec_ap->GetFileSpecAtIndex(i);
----------------
The same situation as I've described above.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53094