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

Aleksandr Urakov via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Oct 11 06:26:34 PDT 2018

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.



More information about the lldb-commits mailing list