diff mbox

[COMMITTED] Fix debug/60438 -- i686 stack vs fp operations

Message ID 535B7C07.2090802@mentor.com
State New
Headers show

Commit Message

Tom de Vries April 26, 2014, 9:27 a.m. UTC
On 13-03-14 21:49, Richard Henderson wrote:
>   (define_expand "ldexpxf3"
> -  [(set (match_dup 3)
> -	(float:XF (match_operand:SI 2 "register_operand")))
> -   (parallel [(set (match_operand:XF 0 " register_operand")
> -		   (unspec:XF [(match_operand:XF 1 "register_operand")
> -			       (match_dup 3)]
> -			      UNSPEC_FSCALE_FRACT))
> -	      (set (match_dup 4)
> -		   (unspec:XF [(match_dup 1) (match_dup 3)]
> -			      UNSPEC_FSCALE_EXP))])]
> +  [(match_operand:XF 0 "register_operand")
> +   (match_operand:XF 1 "register_operand")
> +   (match_operand:SI 2 "register_operand")]
>     "TARGET_USE_FANCY_MATH_387
>      && flag_unsafe_math_optimizations"
>   {
> @@ -14808,6 +14633,11 @@
>
>     operands[3] = gen_reg_rtx (XFmode);
>     operands[4] = gen_reg_rtx (XFmode);
> +
> +  emit_insn (gen_floatsixf2 (operands[3], operands[2]));
> +  emit_insn (gen_fscalexf4_i387 (operands[0], operands[4],
> +                                 operands[1], operands[3]));
> +  DONE;
>   })

Richard,

For a non-bootstrap x86_64 build, gcc.dg/builtins-34.c fails for me with a sigsegv.

I've traced it back to this code in insn-emit.c:
...
rtx
gen_ldexpxf3 (rtx operand0,
         rtx operand1,
         rtx operand2)
{
   rtx _val = 0;
   start_sequence ();
   {
     rtx operands[3];
     operands[0] = operand0;
     operands[1] = operand1;
     operands[2] = operand2;

{
   if (optimize_insn_for_size_p ())
     FAIL;

   operands[3] = gen_reg_rtx (XFmode);
   operands[4] = gen_reg_rtx (XFmode);
...

operands is declared with size 3, and operands[3,4] accesses are out of bounds.

I've done a minimal build with attached patch, and reran the test-case, which 
passes now.

OK if bootstrap succeeds?

Thanks,
- Tom

Comments

Uros Bizjak April 26, 2014, 9:38 a.m. UTC | #1
On Sat, Apr 26, 2014 at 11:27 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 13-03-14 21:49, Richard Henderson wrote:
>>
>>   (define_expand "ldexpxf3"
>> -  [(set (match_dup 3)
>> -       (float:XF (match_operand:SI 2 "register_operand")))
>> -   (parallel [(set (match_operand:XF 0 " register_operand")
>> -                  (unspec:XF [(match_operand:XF 1 "register_operand")
>> -                              (match_dup 3)]
>> -                             UNSPEC_FSCALE_FRACT))
>> -             (set (match_dup 4)
>> -                  (unspec:XF [(match_dup 1) (match_dup 3)]
>> -                             UNSPEC_FSCALE_EXP))])]
>> +  [(match_operand:XF 0 "register_operand")
>> +   (match_operand:XF 1 "register_operand")
>> +   (match_operand:SI 2 "register_operand")]
>>     "TARGET_USE_FANCY_MATH_387
>>      && flag_unsafe_math_optimizations"
>>   {
>> @@ -14808,6 +14633,11 @@
>>
>>     operands[3] = gen_reg_rtx (XFmode);
>>     operands[4] = gen_reg_rtx (XFmode);
>> +
>> +  emit_insn (gen_floatsixf2 (operands[3], operands[2]));
>> +  emit_insn (gen_fscalexf4_i387 (operands[0], operands[4],
>> +                                 operands[1], operands[3]));
>> +  DONE;
>>   })
>
>
> Richard,
>
> For a non-bootstrap x86_64 build, gcc.dg/builtins-34.c fails for me with a
> sigsegv.
>
> I've traced it back to this code in insn-emit.c:
> ...
> rtx
> gen_ldexpxf3 (rtx operand0,
>         rtx operand1,
>         rtx operand2)
> {
>   rtx _val = 0;
>   start_sequence ();
>   {
>     rtx operands[3];
>     operands[0] = operand0;
>     operands[1] = operand1;
>     operands[2] = operand2;
>
> {
>   if (optimize_insn_for_size_p ())
>     FAIL;
>
>   operands[3] = gen_reg_rtx (XFmode);
>   operands[4] = gen_reg_rtx (XFmode);
> ...
>
> operands is declared with size 3, and operands[3,4] accesses are out of
> bounds.
>
> I've done a minimal build with attached patch, and reran the test-case,
> which passes now.
>
> OK if bootstrap succeeds?
>
> 2014-04-26  Tom de Vries  <tom@codesourcery.com>
>
> * config/i386/i386.md (define_expand "ldexpxf3"): Fix out-of-bounds
> array accesses.

OK for mainline and 4.9 branch.

Thanks,
Uros.
Tom de Vries April 26, 2014, 4:42 p.m. UTC | #2
> OK if bootstrap succeeds?

With testing of the bootstrap build of the patch, I ran into the following 
regression compared to a reference bootstrap build without the patch:
...
FAIL: g++.dg/tsan/cond_race.C  -O2  output pattern test, is ==3087==WARNING: 
Program is run with unlimited stack size, which wouldn't work with Threa\
dSanitizer.
==3087==Re-execing with stack size limited to 33554432 bytes.
==================
WARNING: ThreadSanitizer: heap-use-after-free (pid=3087)
   Read of size 8 at 0x7d180000efc8 by thread T1:
     #0 pthread_cond_signal 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.i\
nc:2266 (libtsan.so.0+0x000000039b21)
     #1 thr(void*) 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/gcc/testsuite/g++.dg/tsan/cond_race.C:20 
(cond_race.exe+0x000000001033\
)
   Previous write of size 8 at 0x7d180000efc8 by main thread:
     #0 operator delete(void*) 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/libsanitizer/tsan/tsan_interceptors.cc:592 
(libtsan.so.0+0\
x0000000494b0)
     #1 main 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/gcc/testsuite/g++.dg/tsan/cond_race.C:34 
(cond_race.exe+0x000000000ea0)
   Location is heap block of size 96 at 0x7d180000efa0 allocated by main thread:
     #0 operator new(unsigned long) 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/libsanitizer/tsan/tsan_interceptors.cc:560 
(libtsan.s\
o.0+0x0000000496f2)
     #1 main 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/gcc/testsuite/g++.dg/tsan/cond_race.C:25 
(cond_race.exe+0x000000000e12)
   Thread T1 (tid=3101, running) created by main thread at:
     #0 pthread_create 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/libsanitizer/tsan/tsan_interceptors.cc:877 
(libtsan.so.0+0x0000000\
47c23)
     #1 main 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/gcc/testsuite/g++.dg/tsan/cond_race.C:29 
(cond_race.exe+0x000000000e5a)
SUMMARY: ThreadSanitizer: heap-use-after-free 
/home/vries/gcc_versions/data/test-fix-expand-ldexp/with/src/gcc/testsuite/g++.dg/tsan/cond_race.C:20 
t\
hr(void*)
==================
ThreadSanitizer: reported 1 warnings
, should match ThreadSanitizer: data race.*pthread_cond_signal.*
...

I've found the same failure here: 
http://gcc.gnu.org/ml/gcc-testresults/2014-01/msg00127.html, so I'm assuming 
it's a spurious failure.

I've committed to trunk and 4.9.

Thanks,
- Tom
diff mbox

Patch

2014-04-26  Tom de Vries  <tom@codesourcery.com>

	* config/i386/i386.md (define_expand "ldexpxf3"): Fix out-of-bounds
	array accesses.
---
 gcc/config/i386/i386.md | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 25e2e93..9f103cf 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -14427,15 +14427,16 @@ 
   "TARGET_USE_FANCY_MATH_387
    && flag_unsafe_math_optimizations"
 {
+  rtx tmp1, tmp2;
   if (optimize_insn_for_size_p ())
     FAIL;
 
-  operands[3] = gen_reg_rtx (XFmode);
-  operands[4] = gen_reg_rtx (XFmode);
+  tmp1 = gen_reg_rtx (XFmode);
+  tmp2 = gen_reg_rtx (XFmode);
 
-  emit_insn (gen_floatsixf2 (operands[3], operands[2]));
-  emit_insn (gen_fscalexf4_i387 (operands[0], operands[4],
-                                 operands[1], operands[3]));
+  emit_insn (gen_floatsixf2 (tmp1, operands[2]));
+  emit_insn (gen_fscalexf4_i387 (operands[0], tmp2,
+                                 operands[1], tmp1));
   DONE;
 })
 
-- 
1.8.3.2