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

[aganea]

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.

[rnk]

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.

[dexonsmith]

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]

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.

[rnk]

For reference, this is the code sequence that needs the SmallVector change:
https://github.com/llvm/llvm-project/blob/99e2cba219aea80b3f11de2aa4e0192b28852de4/clang/lib/Frontend/CompilerInvocation.cpp#L1872

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]

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]

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]

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

[dblaikie]

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]

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]

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]

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.

clang/include/clang/Frontend/FrontendOptions.h

public:
   };
   unsigned ObjCMTAction = ObjCMT_None;
   std::string ObjCMTWhiteListPath;
 
   std::string MTMigrateDir;
   std::string ARCMTMigrateReportOut;
 
   /// The input files and their types.
-  std::vector<FrontendInputFile> Inputs;
+  SmallVector<FrontendInputFile, 0> Inputs;
 
   /// When the input is a module map, the original module map file from which
   /// that map was inferred, if any (for umbrella modules).
   std::string OriginalModuleMap;
 
   /// The output file, if any.
   std::string OutputFile;