Re: [PATCH][Middle-end]2nd patch of PR78809 and PR83026

[Qing Zhao <qing dot zhao at oracle dot com>]

> On Dec 15, 2017, at 11:47 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Fri, Dec 15, 2017 at 11:17:37AM -0600, Qing Zhao wrote:
>>  HOST_WIDE_INT const_string_leni = -1;
>> 
>>  if (idx1)
>>    {
>>      const_string_leni = compute_string_length (idx1);
>>      var_string = arg2;
>>    }
>>  else if (idx2)
>>    {
>>      const_string_leni = compute_string_length (idx2);
>>      var_string = arg1;
>>    }
>> 
>> so, the -Wmaybe-uninitialized should NOT issue warning, right?
> 
> Well, you had the var_string var uninitialized, so that is what I was
> talking about.

oh, yeah, forgot to initialize var_string.
> 
>> but anyway, I can change the above as following:
>> 
>> HOST_WIDE_INT const_string_leni = -1;
> 
> And here you don't need to initialize it.
> 
>>  if (idx1)
>>    {
>>      const_string_leni = compute_string_length (idx1);
>>      var_string = arg2;
>>    }
>>  else
>>    {
>>      gcc_assert (idx2);
>>      const_string_leni = compute_string_length (idx2);
>>      var_string = arg1;
>>    }
>> 
>> is this better?
> 
> Yes, though the gcc_assert could be just gcc_checking_assert (idx2);

what’s the major difference between these two assertion calls?

> 
>>> so that would be mode 2, though that
>>> mode isn't actually used in real-world code and thus might be not fully
>>> tested.
>> 
>> so, using this routine with mode 2 should be the right approach to go? 
>> and we need fully testing on this too?
> 
> It has been a while since I wrote it, so it would need careful analysis.
> 
>>>> 	B. use “get_range_strlen” in gimple-fold.h to decide the size of the object.  however, 
>>>> it cannot provide valid info for simple array, either. 
>>> 
>>> get_range_strlen returns you a range, the minval is not what you're looking
>>> for, that is the minimum string length, so might be too short for your
>>> purposes.  And maxval is an upper bound, but you are looking for lower
>>> bound, you need guarantees this amount of memory can be accessed, even if
>>> there is 0 in the first byte.
>> 
>> my understanding is that: get_range_strlen returns the minimum and maximum length of the string pointed by the 
>> pointer, and the maximum length of the string is determined by the size of the allocated memory pointed by the
>> pointer, so, it should serve my purpose,   did I misunderstand it?
> 
> What I'm worried about is:
> struct S { int a; char b[64]; };
> struct T { struct S c; char d; };
> int
> foo (struct T *x)
> {
>  return strcmp (x->c.b, "01234567890123456789012345678901234567890123456789") == 0;
> }
> int
> bar (void)
> {
>  struct S *p = malloc (offsetof (struct S, b) + 8);
>  p->a = 123;
>  strcpy (p->b, "0123456");
>  return foo ((struct T *) p);
> }
> etc.  where if you transform that into memcmp (x->c.b, "01234567890123456789012345678901234567890123456789", 51) == 0
> it will segfault, whereas strcmp would not.

thanks for the example.

is this issue only for the flexible array member? 
if so, the current get_range_strlen will distinguish whether this is for flexible array member or not, then we can disable such transformation
for flexible array member.


> 
>>> But if we find out during
>>> expansion we don't want to expand it inline, we should fall back to calling
>>> strcmp or strncmp.
>> 
>> under what situation we will NOT expand the memcpy_eq call inline?
> 
>      target = expand_builtin_memcmp (exp, target, fcode == BUILT_IN_MEMCMP_EQ);
>      if (target)
>        return target;
>      if (fcode == BUILT_IN_MEMCMP_EQ)
>        {
>          tree newdecl = builtin_decl_explicit (BUILT_IN_MEMCMP);
>          TREE_OPERAND (exp, 1) = build_fold_addr_expr (newdecl);
>        }
> is what builtins.c has, so it certainly counts with the possibility.
> Now, both expand_builtin_memcmp, and emit_block_cmp_hints has several cases
> when it fails.  E.g. can_do_by_pieces decides it is too expensive to do it
> inline, and emit_block_cmp_via_cmpmem fails because the target doesn't have
> cmpmemsi expander.  Various other cases.
> 
> Also, note that some target might have cmpstr*si expanders implemented, but
> not cmpmemsi, in which case trying to optimize strcmp as memcmp_eq might be a
> severe pessimization.

Okay, I see.  this is reasonable. 

if the following better:  (some details still need more work, just basic idea)

in handle_builtin_string_cmp of tree-ssa-strlen.c:

+  /* Replace strcmp or strncmp with the BUILT_IN_STRCMP_EQ.  */
+  if (var_sizei > final_length) 
+    {
+      tree fn = builtin_decl_implicit (BUILT_IN_STRCMP_EQ/BUILT_IN_STRNCMP_EQ;
+      if (!fn)
+	return true;
+      tree const_string_len = build_int_cst (size_type_node, final_length); 
+      update_gimple_call (gsi, fn, 3, arg1, arg2, const_string_len);
+    }

then in builtins.c, add the following:
    case BUILT_IN_STRCMP_EQ:
    case BUILT_IN_STRNCMP_EQ:
      target = expand_builtin_memcmp (exp, target, fcode == BUILT_IN_MEMCMP_EQ);
      if (target)
        return target;
      else 
        {
         tree newdecl = builtin_decl_explicit (BUILT_IN_STRCMP/BUILT_IN_STRNCMP);
          TREE_OPERAND (exp, 1) = build_fold_addr_expr (newdecl);
        }
      
      break;


thanks.

Qing

> 
> 	Jakub