[PATCH] D25858: [tsan][llvm] Implement the function attribute to disable TSan checking at run time

[Anna Zaks via llvm-commits]

zaks.anna added inline comments.


================
Comment at: lib/Transforms/Instrumentation/ThreadSanitizer.cpp:410
+  }
+  for (auto NoRetCallInst : NoRetCalls) {
+    IRBuilder<> IRB(NoRetCallInst);
----------------
dvyukov wrote:
> zaks.anna wrote:
> > dvyukov wrote:
> > > Do you have any particular use case for noreturn calls? I don't think it will work as expected. We don't return from that function, not from this function. Consider that we longjmp solely within this function, we will still exit from this function and call TsanIgnoreEnd on normal return path.
> > > Moreover, most of the functions that recursively call noreturn function are not noreturn themselves. So if we call a function that recursively calls longjmp, we will fail to call TsanIgnoreEnd.
> > > 
> > One of my reduced test cases was calling 'exit()' inside dealloc, which lead to the runtime warning about mismatched ignores. This code fixes that case. Given that, I do not think it's likely that anyone would call a no-return function from dealloc.
> > 
> > I see that this fix will not work in all cases as you pointed out. Would you prefer removing it altogether or adding a comment explaining that it won't work in general?
> For exit() we probably should fix runtime to not complain in such case (when thread is terminated due to exit), because it is not related this functionality, user can write something along the following lines:
> 
> ...
> ANNOTATE_IGNORE_READS_BEGIN();
> ...
> if (some_error) {
>   printf(...);
>   exit(1);
> }
> ...
> ANNOTATE_IGNORE_READS_END();
> ...
> 
> Producing a warning in such case does not look particularly useful.
> However, when main returns with ignores enabled we should still produce a warning, because that means that the program potentially runs with ignores enabled all the time due to a mismatched BEGIN/END somewhere.
> 
> Then I would prefer to remove special handling of noreturn functions, because it's not working.
> 
> We could handle ignores the same way we handle shadow stack for noreturn functions. I.e. longjmp unwinds shadow stack and restores some other internal state, it could also restore ignore counter. 
Makes sense! I'll remove the handling of noreturn from this patch.


https://reviews.llvm.org/D25858