Page MenuHomePhabricator

[C-index] Fix test when using Debug target & MSVC STL
ClosedPublic

Authored by aganea on Nov 7 2019, 11:50 AM.

Details

Summary

This is an attempt to fix clang/test/Index/crash-recovery-modules.m when compiling a build with the MSVC STL & _ITERATOR_DEBUG_LEVEL == 2 (meaning a DEBUG build)
The problem is related to the STL implementation.
I do run sometimes the Debug build with LLVM_ENABLE_ASSERTIONS (on x64), it brings up interesting issues (see rG007d173e2e0c29903bc17a9d5108f531a6f2ea4d)

The problem

Before, the test used to freeze with this callstack:

A lock was taken by another thread that died since.

The test forcibly triggers a crash with the help of #pragma clang __debug crash. This would hit clang/lib/Lex/Pragma.cpp, L1040.
However the call was made through a CrashRecoveryContext, which is essentially a try/catch, so we get back at clang/lib/Frontend/CompilerInstance.cpp, L1152:

However, later on at the end of this function, CompilerInvocation is destroyed, which in turn triggers destruction of FrontendOptions, which eventually triggers the destruction of its Inputs member.

The problem here is that Inputs was being iterated just before the #pragma crash occurred, and CrashRecoveryContext uses SEH. This leaves STL's internal linked list (for the Inputs vector) in a stale state, pointing to a stack variable that doesn't exist anymore (now that we got out of CrashRecoveryContext). When Inputs is being destroyed, the code in C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.16.27023\include\xutility tried to clear that linked list. But now we have nodes pointing to stack memory pages that were already un-commited.

All this ends up to a second crash which happens to leave the _LOCK_DEBUG mutex locked. The crash itself is protected by another CrashRecoveryContext invoked by clang/tools/libclang/CIndex.cpp, L3624 so the program doesn't crash. But it now ends in a stale state, and the _LOCK_DEBUG mutex is locked forever, which leads to the freeze in the top screenshot.

Diff Detail

Event Timeline

aganea created this revision.Nov 7 2019, 11:50 AM
aganea edited the summary of this revision. (Show Details)Nov 7 2019, 2:32 PM
aganea edited the summary of this revision. (Show Details)Nov 8 2019, 1:01 PM
rnk added a comment.Nov 11 2019, 3:37 PM

I see. You know, these things would be nicely handled if we just had a reliable asynch SEH implementation. >_>

What do you think of making FrontendOptions::Inputs be a SmallVector<*, 0>? It's janky and inconsistent with the rest of that struct, but it seems less invasive in the end.

If we ever implement iterator invalidation checks for SmallVector, I expect we'll use some kind of epoch counting strategy that hopefully won't require locking. I assume it would be similar to what is done for DenseMap.

In D69959#1741456, @rnk wrote:

What do you think of making FrontendOptions::Inputs be a SmallVector<*, 0>? It's janky and inconsistent with the rest of that struct, but it seems less invasive in the end.

+1, we use SmallVector<*,0> elsewhere to make up for deficiencies in std::vector. If you're changing Inputs, should you change the rest of the std::vectors in that struct for consistency? There isn't really a downside (on the contrary, it saves a word each).

aganea updated this revision to Diff 228971.Nov 12 2019, 3:04 PM
aganea edited the summary of this revision. (Show Details)

Using SmallVector as requested.

The change in SmallVector.h is to support things such as Opts.AddPluginActions = Args.getAllArgValues(OPT_add_plugin); where getAllArgValues() returns a std::vector.

Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 3:04 PM
rnk added a subscriber: dblaikie.Nov 12 2019, 3:24 PM
clang-tools-extra/clang-move/tool/ClangMove.cpp
113 ↗(On Diff #228971)

Converting from std::list to SmallVector by way of std::vector with a C-style cast is a bit surprising. Is there a simpler way to write this, like Spec.Names.assign(Names.begin(), Names.end());?

llvm/include/llvm/ADT/SmallVector.h
905 ↗(On Diff #228971)

+@dblaikie, who knows more about STL container details than I do.

dblaikie added inline comments.Nov 12 2019, 5:39 PM
clang-tools-extra/clang-move/tool/ClangMove.cpp
113 ↗(On Diff #228971)

Yep, +1 to assign(begin, end) & leaving SmallVector's API alone/not adding a converting assignment operator.

aganea marked 3 inline comments as done.Nov 13 2019, 10:48 AM
aganea added inline comments.
clang-tools-extra/clang-move/tool/ClangMove.cpp
113 ↗(On Diff #228971)

The RHS Names is a cl::list which defines a cast operator for std::vector<DataType>, see here.
However I agree it is a bit surprising, it'd maybe better if that was a real function, so we could write Names.getVec() instead.

llvm/include/llvm/ADT/SmallVector.h
905 ↗(On Diff #228971)

This assignment was added to support converting FrontendOptions's members from std::vector -> SmallVector as suggested by @dexonsmith. As stated above, this was to support code such as:

Opts.ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);

Which otherwise wouldn't work, as getAllArgValues() returns a std::vector.

aganea updated this revision to Diff 229136.EditedNov 13 2019, 10:49 AM

Revert everything, but the change to FrontendOptions::Inputs.

dblaikie added inline comments.Nov 13 2019, 11:04 AM
llvm/include/llvm/ADT/SmallVector.h
905 ↗(On Diff #228971)

I agree with @dexonsmith's suggestion/agreement with @rnk's suggestion to use SmallVector - but to implement the suggestion I'd rather change this:

Opts.ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);

to this:

const cl::list &ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);
Opts.ASTMergeFiles.assign(ASTMergeFiles.begin(), ASTMergeFiles.end());

As @rnk was mentioning. (alternatively, I suppose, since cl::list already has implicit conversion to std::vector, perhaps adding an implicit conversion to SmallVector /there/ would be consistent/not too bad/better than adding std::vector conversion to SmallVector to SmallVector itself)

aganea marked an inline comment as done.Nov 13 2019, 11:17 AM
aganea added inline comments.
llvm/include/llvm/ADT/SmallVector.h
905 ↗(On Diff #228971)

Can we do that in a subsequent patch? This current patch as it stand fixes the issue mentioned in the summary.

dblaikie added inline comments.Nov 13 2019, 3:04 PM
llvm/include/llvm/ADT/SmallVector.h
905 ↗(On Diff #228971)

I'm OK with the narrower fix - changing only the specific container & leaving the rest. I'd probably suggest putting a comment explaining why this is done.

I'll leave it up to @rnk for the final ack.

rnk accepted this revision.Nov 14 2019, 10:29 AM

lgtm

Related to this discussion, I noticed to_vector in STLExtras.h:
https://github.com/llvm/llvm-project/blob/e2369fd197d9e/llvm/include/llvm/ADT/STLExtras.h#L1329
I guess that would be the most idiomatic way to do this if we needed it.

This revision is now accepted and ready to land.Nov 14 2019, 10:29 AM
This revision was automatically updated to reflect the committed changes.