dxilconv: Concurrent first calls to IDxbcConverter::Convert cause a race during LLVM pass registration

[damyanp]

Description

A race can occur when many threads call IDxbcConverter::Convert at the same time. The race occurs after a new process loads dxilconv.dll.

On Windows, PassRegistry does not use its reader and writer locks. The code assumes that the DLL registers all passes before concurrent conversion starts.

However, DXBC conversion registers some passes when it first uses them. For example, DxilCleanup::RemoveDeadCode calls createDeadCodeEliminationPass(). The pass constructor calls initializeDCEPass().

Therefore, two or more conversion threads can call PassRegistry::registerPass() at the same time. Other threads can also call PassRegistry::getPassInfo() during the registration.

A Debug build detects this race with an assertion in CheckThreadId(). An optimized build can read invalid DenseMap state and cause an access violation.

Reproduction procedure

The regression test uses a test DLL and a helper process.

The helper process does these steps:

1. Compile 256 different Shader Model 5 pixel shaders to DXBC.
2. Load the dxilconv.dll file from the helper directory.
3. Get the address of DxcCreateInstance.
4. Start 256 worker threads.
5. Create one IDxbcConverter object on each worker thread.
6. Wait until all worker threads are ready.
7. Release all worker threads at the same time.
8. Call IDxbcConverter::Convert once on each worker thread.

Use different DXBC inputs for the worker threads. Tests that use the same DXBC input do not reproduce the race reliably.

Each worker thread runs this code:

ComPtr<IDxbcConverter> converter;
createInstance(
    CLSID_DxbcConverter,
    __uuidof(IDxbcConverter),
    reinterpret_cast<void **>(converter.GetAddressOf()));

ready.fetch_add(1, std::memory_order_release);
WaitForSingleObject(start, INFINITE);

void *dxil = nullptr;
UINT32 dxilSize = 0;
LPWSTR diagnostics = nullptr;

HRESULT result = converter->Convert(
    shaders[threadIndex]->GetBufferPointer(),
    static_cast<UINT32>(shaders[threadIndex]->GetBufferSize()),
    nullptr,
    &dxil,
    &dxilSize,
    &diagnostics);

CoTaskMemFree(dxil);
CoTaskMemFree(diagnostics);

The TAEF test starts the helper in a new process. A new process is necessary because pass initialization has process scope. Later attempts in the same process do not test the first initialization.

Build the repository with HLSL_BUILD_DXILCONV and tests enabled. Then, run this TAEF test:

DxilConvPassRegistryTest::ConcurrentInitialization

Actual result

In a Debug build, the first helper process stops with exception 0xE0000001.

This assertion fails:

assert(g_PassRegistryTid == GetCurrentThreadId() &&
       "else updating PassRegistry from incorrect thread")

The debugger shows this call stack:

dxilconv!llvm_assert
dxilconv!CheckThreadId
dxilconv!llvm::PassRegistry::registerPass
dxilconv!initializeDCEPassOnce
dxilconv!llvm::initializeDCEPass
dxilconv!DCE::DCE
dxilconv!llvm::createDeadCodeEliminationPass
dxilconv!DxilCleanupNS::DxilCleanup::RemoveDeadCode
dxilconv!DxilCleanupNS::DxilCleanup::runOnModule
dxilconv!llvm::legacy::PassManager::run
dxilconv!hlsl::DxbcConverter::Optimize
dxilconv!hlsl::DxbcConverter::ConvertImpl
dxilconv!hlsl::DxbcConverter::Convert

The assertion is in lib/IR/PassRegistry.cpp:

static void CheckThreadId() {
  if (g_PassRegistryTid == 0)
    g_PassRegistryTid = GetCurrentThreadId();
  else
    assert(g_PassRegistryTid == GetCurrentThreadId() &&
           "else updating PassRegistry from incorrect thread");
}

Optimized builds can fail with this access violation:

0xC0000005: Access violation reading location 0x0000000000000008

dxilconv!llvm::PassRegistry::getPassInfo
dxilconv!llvm::Pass::getPassName
dxilconv!llvm::FPPassManager::runOnFunction
dxilconv!IsReducible
dxilconv!hlsl::DxbcConverter::Optimize
dxilconv!hlsl::DxbcConverter::Convert

The optimized failure depends on thread timing. One repository-built RelWithDebInfo test completed 200 attempts without an access violation. The Debug assertion fails reliably and confirms the concurrent registry update.

Expected result

Concurrent calls to IDxbcConverter::Convert must not update the global pass registry without synchronization.

The calls must not cause an assertion, a deadlock, registry corruption, or an access violation.

Cause

On Windows, PassRegistry::getPassInfo() does not use the reader lock:

const PassInfo *PassRegistry::getPassInfo(const void *TI) const {
#ifndef LLVM_ON_WIN32
  sys::SmartScopedReader<true> Guard(Lock);
#endif
  MapType::const_iterator I = PassInfoMap.find(TI);
  return I != PassInfoMap.end() ? I->second : nullptr;
}

On Windows, PassRegistry::registerPass() uses CheckThreadId() instead of a lock:

void PassRegistry::registerPass(const PassInfo &PI, bool ShouldFree) {
#ifdef LLVM_ON_WIN32
  CheckThreadId();
#else
  sys::SmartScopedReader<true> Guard(Lock);
#endif
  PassInfoMap.insert(...);
}

The code assumes that pass registration is complete before concurrent work starts. This assumption is not correct for the passes that DxilCleanup creates when it first uses them.

Possible corrections

One correction is to register all passes before concurrent conversion starts. This registration must include DCE and all other passes that the DXBC conversion path can create.

Another correction is to add synchronization for Windows PassRegistry readers and writers.

Keep the regression test in a separate helper process. This design makes sure that each test starts with a new pass registry.

Related issues

Issue #79 reports concurrent shader compilation during lazy PassRegistry::registerPass() calls. The discussion recommends registration of all passes before compilation starts.

Issue #4792 reports a different concurrency failure during lazy LLVM pass initialization.

This issue applies specifically to the IDxbcConverter::Convert and DxilCleanup path.

Environment

• Windows x64
• DirectXShaderCompiler windows-ub
• Commit 864cf79541e1f2975837c2828a75174e4c91b1b2
• Debug build: the assertion fails reliably
• Optimized build: the access violation depends on thread timing