[PATCH][SH] Check for 0 length with inlined strnlen builtin

[Christian Bruel]

Hello,

We should not enter the first iteration when length is 0. Testcase 
attached. Difficult to reduce because register allocation generated 
accidentally the correct return value.


testsuite OK

OK for 4.9 and trunk ?

Christian

2015-01-08  Christian Bruel  

	PR target/64507
	* config/sh/sh-mem.cc (sh_expand_cmpnstr): Check 0 length.

2015-01-08  Christian Bruel  

	PR target/64507
	* gcc.target/sh/pr64507.c: New test.

Index: gcc/config/sh/sh-mem.cc
===
--- gcc/config/sh/sh-mem.cc	(revision 219182)
+++ gcc/config/sh/sh-mem.cc	(working copy)
@@ -1,5 +1,5 @@
 /* Helper routines for memory move and comparison insns.
-   Copyright (C) 2013-2014 Free Software Foundation, Inc.
+   Copyright (C) 2013-2015 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -421,6 +421,7 @@
 	  /* end loop.  Reached max iterations.  */
 	  if (sbytes == 0)
 	{
+	  emit_insn (gen_subsi3 (operands[0], tmp1, tmp2));
 	  jump = emit_jump_insn (gen_jump_compact (L_return));
 	  emit_barrier_after (jump);
 	}
@@ -496,6 +497,13 @@
   jump = emit_jump_insn (gen_jump_compact( L_end_loop_byte));
   emit_barrier_after (jump);
 }
+  else
+{
+  emit_insn (gen_tstsi_t (len, len));
+  emit_move_insn (operands[0], const0_rtx);
+  jump = emit_jump_insn (gen_branch_true (L_return));
+  add_int_reg_note (jump, REG_BR_PROB, prob_unlikely);
+}
 
   addr1 = adjust_automodify_address (addr1, QImode, s1_addr, 0);
   addr2 = adjust_automodify_address (addr2, QImode, s2_addr, 0);
@@ -536,10 +544,10 @@
 emit_insn (gen_zero_extendqisi2 (tmp2, gen_lowpart (QImode, tmp2)));
   emit_insn (gen_zero_extendqisi2 (tmp1, gen_lowpart (QImode, tmp1)));
 
-  emit_label (L_return);
-
   emit_insn (gen_subsi3 (operands[0], tmp1, tmp2));
 
+  emit_label (L_return);
+
   return true;
 }
 
Index: gcc/testsuite/gcc.target/sh/pr64507.c
===
--- gcc/testsuite/gcc.target/sh/pr64507.c	(revision 0)
+++ gcc/testsuite/gcc.target/sh/pr64507.c	(working copy)
@@ -0,0 +1,25 @@
+/* Check that the __builtin_strnlen returns 0 with with 
+   non-constant 0 length.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+extern int snprintf(char *, int, const char *, ...);
+extern void abort (void);
+
+int main()
+ {
+   int i;
+   int cmp = 0;
+   char buffer[1024];
+   const char* s = "the string";
+
+   snprintf(buffer, 4, "%s", s);
+
+   for (i = 1; i < 4; i++)
+ cmp += __builtin_strncmp(buffer, s, i - 1);
+
+  if (cmp)
+abort();
+
+  return 0;
+}

Re: [PATCH][SH] Check for 0 length with inlined strnlen builtin

[Oleg Endo]

On Tue, 2015-01-06 at 10:28 +0100, Christian Bruel wrote:
> Hello,
> 
> We should not enter the first iteration when length is 0. Testcase 
> attached. Difficult to reduce because register allocation generated 
> accidentally the correct return value.
> 
> testsuite OK
> 
> OK for 4.9 and trunk ?

In your patch:

+  emit_insn (gen_tstsi_t (len, len));

Please use 'gen_cmpeqsi_t (len, const0_rtx)' for comparing a value
against zero instead of the bit test insn.  Otherwise, I think it's OK
for trunk and 4.9.

Cheers,
Oleg

Re: [PATCH][SH] Check for 0 length with inlined strnlen builtin

[Christian Bruel]

Please use 'gen_cmpeqsi_t (len, const0_rtx)' for comparing a value
against zero instead of the bit test insn.


OK, also then OK to replace the other occurrences of the idiom for 
coding consistency ? (not sure if I could commit this as obvious ?).


Cheers

Christian




2015-01-08  Christian Bruel  

	* config/sh/sh-mem.cc (sh_expand_cmpnstr, sh_expand_setmem):
	 Use gen_cmpeqsi instead of gen_tstsi for comparing against 0.

Index: gcc/config/sh/sh-mem.cc
===
--- gcc/config/sh/sh-mem.cc	(revision 219257)
+++ gcc/config/sh/sh-mem.cc	(working copy)
@@ -410,7 +410,7 @@
 	  else
 	{
 	  emit_insn (gen_addsi3 (lenw, lenw, GEN_INT (-1)));
-	  emit_insn (gen_tstsi_t (lenw, lenw));
+ emit_insn (gen_cmpeqsi_t (lenw, const0_rtx));
 	}
 
 	  jump = emit_jump_insn (gen_branch_false (L_loop_long));
@@ -531,7 +531,7 @@
   else
 {
   emit_insn (gen_addsi3 (len, len, GEN_INT (-1)));
-  emit_insn (gen_tstsi_t (len, len));
+  emit_insn (gen_cmpeqsi_t (len, const0_rtx));
 }
 
   jump = emit_jump_insn (gen_branch_false (L_loop_byte));
@@ -691,7 +691,7 @@
   else
 	{
 	  emit_insn (gen_addsi3 (lenw, lenw, GEN_INT (-1)));
-	  emit_insn (gen_tstsi_t (lenw, lenw));
+ emit_insn (gen_cmpeqsi_t (lenw, const0_rtx));
 	}
 
   emit_move_insn (dest, val);
@@ -728,7 +728,7 @@
   else
 {
   emit_insn (gen_addsi3 (len, len, GEN_INT (-1)));
-  emit_insn (gen_tstsi_t (len, len));
+  emit_insn (gen_cmpeqsi_t (len, const0_rtx));
 }
 
   val = gen_lowpart (QImode, val);

Re: [PATCH][SH] Check for 0 length with inlined strnlen builtin

[Oleg Endo]

On Tue, 2015-01-06 at 13:13 +0100, Christian Bruel wrote:
> > Please use 'gen_cmpeqsi_t (len, const0_rtx)' for comparing a value
> > against zero instead of the bit test insn.
> 
> OK, also then OK to replace the other occurrences of the idiom for 
> coding consistency ? (not sure if I could commit this as obvious ?).

In the end the same "tst reg,reg" instruction will be generated, but
with one reg operand instead of two.  It might have some impact on the
register allocation in some cases.  If it passes testing, it's OK I
think.

Cheers,
Oleg