[PATCH] D26177: [tsan] Add support for C++ exceptions into TSan (call __tsan_func_exit during unwinding)

[Reid Kleckner via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D26177#585299, @kubabrecka wrote:

> Also note that this change increases the code size of TSanified binaries, roughly by 10-20%.


I don't expect there will be any increase in code size if -fno-exceptions is set, because all functions will be marked nounwind, so we won't do the call-to-invoke transformation.



================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:30-31
+///
+/// It's wrapped up in a state machine using the same transform C# uses for
+/// 'yield return' enumerators, This transform allows it to be non-allocating.
+class EscapeEnumerator {
----------------
The state machine is superfluous, it only has two real states: 1 (running) and 2 (done). The state 0 logic could be handled in the constructor by setting StateBB and StateE. Mind killing the switch and this comment, assuming we don't implement the code size optimization I described below?


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:45
+
+  IRBuilder<> *Next() {
+    switch (State) {
----------------
The implementation of Next should not live in a header.


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:54
+      State = 1;
+
+    case 1:
----------------
LLVM_FALLTHROUGH would help me understand what's supposed to happen here.


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:74-76
+      for (Function::iterator BB = F.begin(), E = F.end(); BB != E; ++BB)
+        for (BasicBlock::iterator II = BB->begin(), EE = BB->end(); II != EE;
+             ++II)
----------------
Range-based for?


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:78-79
+          if (CallInst *CI = dyn_cast<CallInst>(II))
+            if (!CI->getCalledFunction() ||
+                !CI->getCalledFunction()->getIntrinsicID())
+              Calls.push_back(CI);
----------------
The right way to ask this is `!CI->doesNotThrow()`. I'm surprised we don't have a non-virtual override of `Instruction::mayThrow` in `CallInst`, but whatever.


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:92
+        Constant *PersFn = F.getParent()->getOrInsertFunction(
+            "__gcc_personality_v0",
+            FunctionType::get(Type::getInt32Ty(C), true));
----------------
This isn't going to be right everywhere. You probably want to add some function to `lib/Analysis/EHPersonalities.cpp` to pick a good default based on the target triple.


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:97
+      LandingPadInst *LPad =
+          LandingPadInst::Create(ExnTy, 1, "cleanup.lpad", CleanupBB);
+      LPad->setCleanup(true);
----------------
A landingpad instruction would be wrong on Windows, you would want to insert a cleanuppad that unwinds to the caller. For now I think it would be OK to report_fatal_error if `isFuncletEHPersonality(classifyEHPersonality(PersFn))` is true.


================
Comment at: include/llvm/Transforms/Utils/EscapeEnumerator.h:122
+        InvokeInst *II =
+            InvokeInst::Create(CI->getCalledValue(), NewBB, CleanupBB, Args,
+                               CI->getName(), CallBB);
----------------
This fails to handle operand bundles on calls. We should factor this logic out of the inliner, which gets it 100% correct, into `lib/Transforms/Util/Local.cpp` next to changeToCall, and use it from here.


================
Comment at: lib/Transforms/Instrumentation/ThreadSanitizer.cpp:448-449
+    EscapeEnumerator EE(F, "tsan_cleanup");
+    while (IRBuilder<> *AtExit = EE.Next()) {
+      AtExit->CreateCall(TsanFuncExit, {});
     }
----------------
This is probably outside the scope of this patch, but is this really what you want to do? Instead of inserting a call before every return, you could create a new single return block with only one call. That might be better for code size. The same is true for resumes: you can make all resumes a jump to the same resume block, and then you only need one exit call, for a total of two calls per function, max.

Actually, this gives me a maybe too clever idea for code size reduction... what if we had __tsan_func_exit check if we are unwinding and call _Unwind_Resume from there? We'd still have binary size increase from the LSDA and we probably couldn't eliminate the landingpad basic block, but it's an interesting idea.


Repository:
  rL LLVM

https://reviews.llvm.org/D26177