Reapply "[ORC] Replace ORC's baked-in dependence ... (#163027)" with …

[lhames]

…fixes.

This reapplies c8c86ef, which was reverted in 13ca872 due to bot failures. Those failures were compilation errors exposed when LLVM is built with LLVM_ENABLE_EXPENSIVE_CHECKS=On. They have been fixed in this commit.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[googlewalt]

We're seeing test failures when tsan is enabled. Attached is a scrubbed backtrace with the relevant information: test.log. Can you please take a look?

[lhames]

We're seeing test failures when tsan is enabled. Attached is a scrubbed backtrace with the relevant information: test.log. Can you please take a look?

Hi @googlewalt. I'm traveling at the moment and not in a position to reproduce, but I suspect this is caused by the assert at the top of AsynchronousSymbolQuery::handleComplete. Any chance you could try the following:

diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp
index 8d413a35f5a9..b82d27b096e6 100644
--- a/llvm/lib/ExecutionEngine/Orc/Core.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp
@@ -230,8 +230,12 @@ void AsynchronousSymbolQuery::notifySymbolMetRequiredState(
 }
 
 void AsynchronousSymbolQuery::handleComplete(ExecutionSession &ES) {
-  assert(OutstandingSymbolsCount == 0 &&
+#ifndef NDEBUG
+  ES.runSessionLocked([this]() {
+    assert(OutstandingSymbolsCount == 0 &&
          "Symbols remain, handleComplete called prematurely");
+  });
+#endif // NDEBUG
 
   class RunQueryCompleteTask : public Task {
   public:

[rupprecht]

The race is from decrementing OutstandingSymbolsCount here:

llvm-project/llvm/lib/ExecutionEngine/Orc/Core.cpp

Line 229 in 8c29bce

--OutstandingSymbolsCount;

Which happens after a different thread reads it as part of UQ->isComplete():

llvm-project/llvm/lib/ExecutionEngine/Orc/Core.cpp

Lines 3046 to 3048 in 8c29bce

	for (auto &UQ : EmitQueries->Updated)
	if (UQ->isComplete())
	UQ->handleComplete(*this);

Previously, that call to isComplete() was only in an assert (assert(Q->isComplete() && "Q is not complete")), so I wonder if this is exposing an existing race issue -- we only run these tests in tsan mode w/ assertions disabled.

[rupprecht]

Previously, that call to isComplete() was only in an assert (assert(Q->isComplete() && "Q is not complete")), so I wonder if this is exposing an existing race issue -- we only run these tests in tsan mode w/ assertions disabled.

Nope. Prior to this change, tsan tests pass even with that assert changed to this:

  for (auto &Q : *CompletedQueries) {
    if (!Q->isComplete())
      dbgs() << "Q is not complete";
    Q->handleComplete(*this);
  }

So this seems like a new race somehow.

Anyway, at trunk, this fix seems to make the tsan errors go away, although I have no clue if it's a good change:

  runSessionLocked([&]() {
    for (auto &UQ : EmitQueries->Updated)
      if (UQ->isComplete())
        UQ->handleComplete(*this);
  });

WDYT?

[lhames]

Which happens after a different thread reads it as part of UQ->isComplete():...

Ahh. Thanks for identifying that!

Anyway, at trunk, this fix seems to make the tsan errors go away, although I have no clue if it's a good change:

  runSessionLocked([&]() {
    for (auto &UQ : EmitQueries->Updated)
      if (UQ->isComplete())
        UQ->handleComplete(*this);
  });

WDYT?

We don't want that -- query handlers need to be run outside the session lock.

I think we'll just need to filter the EmitQueries->Updated list by Q->IsComplete() at the end of IL_emit just before we return. So that member should become EmitQueries::CompletedQueries, and the loop outside can become:

for (auto &CQ : EmitQueries->CompletedQueries)
  CQ->handleComplete(*this);

[rupprecht]

WDYT?

We don't want that -- query handlers need to be run outside the session lock.

I didn't think so, but it was worth a shot ;)

I think we'll just need to filter the EmitQueries->Updated list by Q->IsComplete() at the end of IL_emit just before we return. So that member should become EmitQueries::CompletedQueries, and the loop outside can become:

for (auto &CQ : EmitQueries->CompletedQueries)
  CQ->handleComplete(*this);

Created #165063 to do that, and it fixes the tsan issues.

[lhames]

Thanks very much for the fix!