[llvm-commits] [PATCH] __builtin_debugtrap() intrinsic function (change to Clang and CodeGen)

[Shuxin Yang]

Hi, Eli:

    Thank you for pointing out these problems.

    Not sure how debugger use __builtin_debugtrap(), I thought it should 
be flagged with "noreturn" like __builtin_trap(). Now I remove the 
attribute.

   There is a subtle problem here. If a programmer replace 
__builtin_trap() with __builtin_debugtrap(), the discrepancy between 
these two functions with regard to the "noreturn" will not incur 
correctness problem. However, replacing __builtin_debugtrap() with 
__builtin_trap() might incur some problems depending on how compiler 
takes advantage of noreturn.

   The attached is the fix to the problems you figure out.

    Thank you again for the code review.

  Shuxin



On 10/19/12 1:28 PM, Eli Friedman wrote:
> On Fri, Oct 19, 2012 at 11:23 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
>> Hi,
>>
>>     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().
>>
>>    The X86 backend is already able to handle debugtrap(). This patch is to:
>>    1) make front-end recognize "__builtin_debugtrap()" (emboddied in the
>> one-line change to Clang).
>>    2) In DAG legalization phase, by default, "debugtrap" will be replaced
>> with "trap", which
>>       make the __builtin_debugtrap() "available" to all existing ports
>> without the hassle of
>>       changing their code.
>>    3) If trap-function is specified (via -trap-func=xyz to llc), both
>> __builtin_debugtrap() and
>>       __builtin_trap() will be expanded into the function call of the
>> specified trap function.
>>      This behavior may need change in the future.
>>
>>    The provided testing-case is to make sure 2) and 3) are working for ARM
>> port, and we
>> already have a testing case for x86.
>>
>>    This change pass regression test, SingleSource and MultipleSource test.
> Index: include/llvm/Intrinsics.td
> ===================================================================
> --- include/llvm/Intrinsics.td	(revision 166287)
> +++ include/llvm/Intrinsics.td	(working copy)
> @@ -420,7 +420,7 @@
>                        GCCBuiltin<"__builtin_flt_rounds">;
>   def int_trap : Intrinsic<[], [], [IntrNoReturn]>,
>                  GCCBuiltin<"__builtin_trap">;
> -def int_debugtrap : Intrinsic<[]>,
> +def int_debugtrap : Intrinsic<[], [], [IntrNoReturn]>,
>                       GCCBuiltin<"__builtin_debugtrap">;
>
> int_debugtrap is intentionally not marked noreturn because you can
> "continue" in a debugger.
>
> +      NewVal = DAG.getNode (ISD::TRAP, Node->getDebugLoc(), Node->getVTList(),
> +                            Node->getOperand(0));
>
> http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses
>
> -Eli

-------------- next part --------------
Index: include/llvm/Intrinsics.td
===================================================================
--- include/llvm/Intrinsics.td	(revision 166300)
+++ include/llvm/Intrinsics.td	(working copy)
@@ -420,7 +420,7 @@
                      GCCBuiltin<"__builtin_flt_rounds">;
 def int_trap : Intrinsic<[], [], [IntrNoReturn]>,
                GCCBuiltin<"__builtin_trap">;
-def int_debugtrap : Intrinsic<[], [], [IntrNoReturn]>,
+def int_debugtrap : Intrinsic<[]>,
                     GCCBuiltin<"__builtin_debugtrap">;
 
 // NOP: calls/invokes to this intrinsic are removed by codegen
Index: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/LegalizeDAG.cpp	(revision 166300)
+++ lib/CodeGen/SelectionDAG/LegalizeDAG.cpp	(working copy)
@@ -1245,8 +1245,8 @@
     if (Action == TargetLowering::Expand) {
       // replace ISD::DEBUGTRAP with ISD::TRAP
       SDValue NewVal;
-      NewVal = DAG.getNode (ISD::TRAP, Node->getDebugLoc(), Node->getVTList(),
-                            Node->getOperand(0));
+      NewVal = DAG.getNode(ISD::TRAP, Node->getDebugLoc(), Node->getVTList(),
+                           Node->getOperand(0));
       ReplaceNode(Node, NewVal.getNode());
       LegalizeOp(NewVal.getNode());
       return;