Skip to content

Commit 8d1ea1c

Browse files
committed
Fix foreign type metadata creation to not call the initializer
under the lock. Unfortunately, unit-testing this is gratuitously difficult because of C++ problems. (Fixed in C++17, apparently.) Will be tested by a future patch which creates a nested foreign type.
1 parent f9c1dd4 commit 8d1ea1c

File tree

1 file changed

+71
-14
lines changed

1 file changed

+71
-14
lines changed

stdlib/public/runtime/Metadata.cpp

Lines changed: 71 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2444,6 +2444,7 @@ struct llvm::DenseMapInfo<GlobalString> {
24442444
namespace {
24452445
struct ForeignTypeState {
24462446
Mutex Lock;
2447+
ConditionVariable InitializationWaiters;
24472448
llvm::DenseMap<GlobalString, const ForeignTypeMetadata *> Types;
24482449
};
24492450
}
@@ -2457,20 +2458,76 @@ swift::swift_getForeignTypeMetadata(ForeignTypeMetadata *nonUnique) {
24572458
return unique;
24582459
}
24592460

2460-
// Okay, insert a new row.
2461-
auto &Foreign = ForeignTypes.get();
2462-
ScopedLock guard(Foreign.Lock);
2463-
2464-
auto insertResult = Foreign.Types.insert({GlobalString(nonUnique->getName()),
2465-
nonUnique});
2466-
auto uniqueMetadata = insertResult.first->second;
2467-
2468-
// If the insertion created a new entry, set up the metadata we were
2469-
// passed as the insertion result.
2470-
if (insertResult.second) {
2471-
// Call the initialization callback if present.
2472-
if (nonUnique->hasInitializationFunction())
2473-
nonUnique->getInitializationFunction()(nonUnique);
2461+
// Okay, check the global map.
2462+
auto &foreignTypes = ForeignTypes.get();
2463+
GlobalString key(nonUnique->getName());
2464+
bool hasInit = nonUnique->hasInitializationFunction();
2465+
2466+
const ForeignTypeMetadata *uniqueMetadata;
2467+
bool inserted;
2468+
2469+
// A helper function to find the current entry for the key using the
2470+
// saved iterator if it's still valid. This should only be called
2471+
// while the lock is held.
2472+
decltype(foreignTypes.Types.begin()) savedIterator;
2473+
size_t savedSize;
2474+
auto getCurrentEntry = [&]() -> const ForeignTypeMetadata *& {
2475+
// The iterator may have been invalidated if the size of the map
2476+
// has changed since the last lookup.
2477+
if (foreignTypes.Types.size() != savedSize) {
2478+
savedSize = foreignTypes.Types.size();
2479+
savedIterator = foreignTypes.Types.find(key);
2480+
assert(savedIterator != foreignTypes.Types.end() &&
2481+
"entries cannot be removed from foreign types metadata map");
2482+
}
2483+
return savedIterator->second;
2484+
};
2485+
2486+
{
2487+
ScopedLock guard(foreignTypes.Lock);
2488+
2489+
// Try to create an entry in the map. The initial value of the entry
2490+
// is our copy of the metadata unless it has an initialization function,
2491+
// in which case we have to insert null as a placeholder to tell others
2492+
// to wait while we call the initializer.
2493+
auto valueToInsert = (hasInit ? nullptr : nonUnique);
2494+
auto insertResult = foreignTypes.Types.insert({key, valueToInsert});
2495+
inserted = insertResult.second;
2496+
savedIterator = insertResult.first;
2497+
savedSize = foreignTypes.Types.size();
2498+
uniqueMetadata = savedIterator->second;
2499+
2500+
// If we created the entry, then the unique metadata is our copy.
2501+
if (inserted) {
2502+
uniqueMetadata = nonUnique;
2503+
2504+
// If we didn't create the entry, but it's null, then we have to wait
2505+
// until it becomes non-null.
2506+
} else {
2507+
while (uniqueMetadata == nullptr) {
2508+
foreignTypes.Lock.wait(foreignTypes.InitializationWaiters);
2509+
uniqueMetadata = getCurrentEntry();
2510+
}
2511+
}
2512+
}
2513+
2514+
// If we inserted the entry and there's an initialization function,
2515+
// call it. This has to be done with the lock dropped.
2516+
if (inserted && hasInit) {
2517+
nonUnique->getInitializationFunction()(nonUnique);
2518+
2519+
// Update the cache entry:
2520+
2521+
// - Reacquire the lock.
2522+
ScopedLock guard(foreignTypes.Lock);
2523+
2524+
// - Change the entry.
2525+
auto &entry = getCurrentEntry();
2526+
assert(entry == nullptr);
2527+
entry = nonUnique;
2528+
2529+
// - Notify waiters.
2530+
foreignTypes.InitializationWaiters.notifyAll();
24742531
}
24752532

24762533
// Remember the unique result in the invasive cache. We don't want

0 commit comments

Comments
 (0)