> On Dec 15, 2017, at 11:47 AM, Jakub Jelinek <ja...@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