[analyzer] Make __builtin_debugtrap() a sink

[xazax.hun]

Summary

For some reason, __builtin_debugtrap is not a sink for the analyzer. I also added some test cases to demonstrate that __builtin_trap and __builtin_unreachable are handled properly. The former is not marked as noreturn while the last two are. See Builtins.def for details. It was made non-noreturn deliberately, see https://reviews.llvm.org/rL166345
I have, however, no idea why.

[xazax.hun]

In the meantime, I found this discussion: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121015/153735.html
It looks like the reasoning behind this intrinsic not being noreturn is that the user might continue the execution in the debugger.
I think the main reason behind not marking this noreturn is to not allow the optimizer to remove code based on the usage of this builtin and thus prohibit debugging certain part of the code.
But it still might make sense to treat this builtin as a sink in the analyzer. What do you think?

[dcoughlin]

I believe that the intent of __builtin_debugtrap() is to provide an in-source mechanism so that the developer can trap into the debugger and then continue execution. For example, in the Swift codebase this is used in combination with a debug flag to break into the debugger at the beginning a specified pass. https://github.com/apple/swift/blob/master/lib/SILOptimizer/PassManager/PassManager.cpp#L339

Since the intent is to continue after the trap, I don't think the analyzer should treat it as a sink.

Did you get requests from users to treat it like a sink? Or false positives that would be resolved if were a sink? Or is this something you noticed by inspection?

[xazax.hun]

In D39551#914360, @dcoughlin wrote:

I believe that the intent of __builtin_debugtrap() is to provide an in-source mechanism so that the developer can trap into the debugger and then continue execution. For example, in the Swift codebase this is used in combination with a debug flag to break into the debugger at the beginning a specified pass. https://github.com/apple/swift/blob/master/lib/SILOptimizer/PassManager/PassManager.cpp#L339

Oh, ok. I think this use is very convincing.

Since the intent is to continue after the trap, I don't think the analyzer should treat it as a sink.

I see. I misinterpreted slightly the intent. From the discussion I linked:

This patch is to fix radar://8426430. It is about llvm support of
__builtin_debugtrap()
which is supposed to consistently raise SIGTRAP across all systems. In
contrast,
__builtin_trap() behave differently on different systems. e.g. it raises
SIGTRAP on ARM, and
SIGILL on X86. The purpose of __builtin_debugtrap() is to consistently
provide "trap"
functionality, in the mean time preserve the compatibility with on gcc on
__builtin_trap().

So according to my interpretation __builtin_debugtrap() is not only a way to express that the user would like to continue with a debugger but also to provide consistent behavior across platforms. Also, I assumed this is mainly used for cases where something already went wrong, so it might make sense to suppress results on that path which might be the side effect of error that triggered to take that path in the first place. But the PassManager example you provided proves this wrong.

Did you get requests from users to treat it like a sink? Or false positives that would be resolved if were a sink? Or is this something you noticed by inspection?

I was just surprised by this behavior when I saw this happening and wanted to make sure whether this is intended. I did not get any user request.

lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

bool BuiltinFunctionChecker::evalCall(const CallExpr *CE,
   const LocationContext *LCtx = C.getLocationContext();
   if (!FD)
     return false;
 
   switch (FD->getBuiltinID()) {
   default:
     return false;
 
+  case Builtin::BI__builtin_debugtrap:
+    C.generateSink(C.getState(), C.getPredecessor());
+    return true;
+
   case Builtin::BI__builtin_assume: {
     assert (CE->arg_begin() != CE->arg_end());
     SVal ArgSVal = state->getSVal(CE->getArg(0), LCtx);
     if (ArgSVal.isUndef())
       return true; // Return true to model purity.
 
     state = state->assume(ArgSVal.castAs<DefinedOrUnknownSVal>(), true);
     // FIXME: do we want to warn here? Not right now. The most reports might

test/Analysis/builtin-functions.cpp

 }
 
 void g(int i) {
   if (i > 5) {
     __builtin_assume(i < 5);
     clang_analyzer_warnIfReached(); // Assumtion contradicts constraints.
                                     // We give up the analysis on this path.
   }
+  switch (i) {
+  case 1:
+    __builtin_unreachable();
+    clang_analyzer_warnIfReached();
+    break;
+  case 2:
+    __builtin_trap();
+    clang_analyzer_warnIfReached();
+    break;
+  case 3:
+    __builtin_debugtrap();
+    clang_analyzer_warnIfReached();
+    break;
+  }
 }