diff mbox

[shrink-wrap] should not sink instructions which may cause trap ?

Message ID 54257C11.9070109@arm.com
State New
Headers show

Commit Message

Jiong Wang Sept. 26, 2014, 2:45 p.m. UTC 
On 26/09/14 09:36, Richard Biener wrote:
> On Fri, Sep 26, 2014 at 12:49 AM, Jiong Wang
> <wong.kwongyuan.tools@gmail.com> wrote:
>> 2014-09-25 14:07 GMT+01:00 Jiong Wang <jiong.wang@arm.com>:
>>> On 25/09/14 12:25, Christophe Lyon wrote:
>>>> I have observed regressions in the g++ testsuite: pr49847 now FAILs
>>>> after this patch.
>>> no.
>>>
>>> even without my patch, the regression still happen.
>>>
>>> or you could specify -fno-shrink-wrap, gcc still crash.
>>>
>>> so, this regression should caused by other commits which haven't exposed on
>>> x86 regression test.
>> sorry, confirmed, there is regression.
>>
>> my code was git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@215590.
>> there also be gcc crash on aarch64, with the following info,
>>    pr49847.C:5:21: internal compiler error: Segmentation fault
>>       try { return g >= 0; }
>>                       ^
>>    0xdc249e crash_signal
>>        ../../gcc/gcc/toplev.c:340
>>    0xdbfeff default_get_reg_raw_mode(int)
>>
>> so I was thinking it's caused by other commits instead of this, and after I sync
>> code to git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@215599 I could
>> reproduce this bug.
>>
>>    error: missing REG_EH_REGION note at the end of bb 2
>>
>> the reson is:
>>    * before this patch, we only sink simple "set reg, reg" instruction which
>>      the corresponding instruction will not produce exception, thus no
>> REG_EH_REGION attached.
>>    * after this patch, we will sink instruction like the following for
>> aarch64 or arm or other RISC.
>>
>>      (insn 7 3 30 2 (set (reg:CCFPE 66 cc)
>>          (compare:CCFPE (reg:SF 32 v0 [ g ])
>>              (const_double:SF 0.0 [0x0.0p+0]))) pr49847.C:5 330 {*cmpesf}
>>       (expr_list:REG_DEAD (reg:SF 32 v0 [ g ])
>>          (expr_list:REG_EH_REGION (const_int 1 [0x1])
>>              (nil))))
>>
>>    "compare" is actually a operator which may cause trap and we need to prevent
>>    any instruction which may causing trap be sink, because that may
>> break exception handling logic
>>
>>    so something like the following should be added to the iterator check
>>
>>    if (may_trap_p (x))
>>      don't sink this instruction.
>>
>>     any comments?
> Should be checking if x may throw internally instead.

Richard, thanks for the suggestion, have used insn_could_throw_p to do the check,
which will only do the check when flag_exception and flag_non_call_exception be true,
so those instruction could still be sink for normal c/c++ program.

Jeff,

   below is the fix for pr49847.C regression on aarch64. I re-run full test on
   aarch64-none-elf bare metal, no regression.

   bootstrap ok on x86, no regression on check-gcc/g++.

   ok for trunk?

   -- Jiong

>
> Richard.
>
>>     I will try to send a fix tomorrow.
>>
>>     thanks.
>>
>> -- Jiong
>>
>>
>>> -- Jiong
>>>
>>>
>>>> Here is what I have in my logs:
>>>>
>>>> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../xg++
>>>>
>>>> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C
>>>> -fno-diagnostics-show-caret -fdiagnostics-color=never  -nostdinc++
>>>>
>>>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include/arm-none-linux-gnueabihf
>>>>
>>>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include
>>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++
>>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward
>>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util
>>>> -fmessage-length=0  -std=gnu++98 -O -fnon-call-exceptions  -S     -o
>>>> pr49847.s    (timeout = 800)
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C: In
>>>> function 'int f(float)':
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1:
>>>> error: missing REG_EH_REGION note at the end of bb 2
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1:
>>>> internal compiler error: verify_flow_info failed
>>>> 0x82f8ba verify_flow_info()
>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfghooks.c:260
>>>>
>>>> 0x840cd3 commit_edge_insertions()
>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgrtl.c:2068
>>>> 0x9bf243 thread_prologue_and_epilogue_insns
>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:5852
>>>> 0x9bfa52 rest_of_handle_thread_prologue_and_epilogue
>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6245
>>>> 0x9bfa52 execute
>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6283
>>>>
>>>> As per
>>>>
>>>> http://cbuild.validation.linaro.org/build/cross-validation/gcc/trunk/215563/report-build-info.html
>>>> I've noticed this on targets:
>>>> arm-none-linux-gnueabihf
>>>> armeb-none-linux-gnueabihf
>>>> aarch64-none-elf
>>>> aarch64_be-none-elf
>>>> aarch64-none-linux-gnu
>>>> but NOT on
>>>> arm-none-eabi
>>>> arm-none-linux-gnueabi
>>>>
>>>> Christophe.
>>>>
>>>
diff mbox

Patch

diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index bd4813c..2e2f0a6 100644
--- a/gcc/shrink-wrap.c
+++ b/gcc/shrink-wrap.c
@@ -189,6 +189,9 @@  move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
       unsigned int nonconstobj_num = 0;
       rtx src_inner = NULL_RTX;

+      if (insn_could_throw_p (insn))
+	return false;
+
       subrtx_var_iterator::array_type array;
       FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
 	{