Re: [PATCH] Convert character arrays to string csts

[Bernd Schmidt <bschmidt at redhat dot com>]

On 11/03/2016 01:12 PM, Martin Liška wrote:
> +  tree init = DECL_INITIAL (decl);
> +  if (init
> +      && TREE_READONLY (decl)
> +      && can_convert_ctor_to_string_cst (init))
> +    DECL_INITIAL (decl) = build_string_cst_from_ctor (init);

I'd merge these two new functions since they're only ever called 
together. We'd then have something like

if (init && TREE_READONLY (decl))
  init = convert_ctor_to_string_cst (init);
if (init)
  DECL_INITIAL (decl) = init;

I'll defer to Jan on whether finalize_decl seems like a good place to do 
this.

> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> index 283bd1c..b2d1fd5 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> @@ -4,12 +4,15 @@
>  char *buffer1;
>  char *buffer2;
>
> +const char global[] = {'a', 'b', 'c', 'd', '\0'};
> +
>  #define SIZE 1000
>
>  int
>  main (void)
>  {
>    const char* const foo1 = "hello world";
> +  const char local[] = "abcd";
>
>    buffer1 = __builtin_malloc (SIZE);
>    __builtin_strcpy (buffer1, foo1);
> @@ -45,6 +48,10 @@ main (void)
>      __builtin_abort ();
>    if (__builtin_memchr (foo1, null, 12) != foo1 + 11)
>      __builtin_abort ();
> +  if (__builtin_memchr (global, null, 5) == 0)
> +    __builtin_abort ();
> +  if (__builtin_memchr (local, null, 5) == 0)
> +    __builtin_abort ();

How is that a meaningful test? This seems to work even with an unpatched 
gcc. I'd have expected something that shows a benefit for doing this 
conversion, and maybe also a test that shows it isn't done in cases 
where it's not allowed.

>  tree
> -build_string_literal (int len, const char *str)
> +build_string_literal (int len, const char *str, bool build_addr_expr)

New arguments should be documented in the function comment.

> +/* Return TRUE when CTOR can be converted to a string constant.  */

"if", not "when".

> +  unsigned HOST_WIDE_INT elements = CONSTRUCTOR_NELTS (ctor);
> +  FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, key, value)
> +    {
> +      if (key == NULL_TREE
> +	  || TREE_CODE (key) != INTEGER_CST
> +	  || !tree_fits_uhwi_p (value)
> +	  || !useless_type_conversion_p (TREE_TYPE (value), char_type_node))
> +	return false;

Shouldn't all elements have the same type, or do you really have to call 
useless_type_conversion in a loop?

> +      /* Allow zero character just at the end of a string.  */
> +      if (integer_zerop (value))
> +	return idx == elements - 1;

Don't you also have to explicitly check it's there?


Bernd