Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2019-09-02 at 09:16 +0000, Andrea Corallo wrote:
> Hi all,
> yesterday I've found an interesting bug in libgccjit.
> Seems we have an hard limitation of 200 characters for literal
> strings.
> Attempting to create longer strings lead to ICE during pass_expand
> while performing a sanity check in get_constant_size.
> 
> Tracking down the issue seems the code we have was inspired from
> c-family/c-common.c:c_common_nodes_and_builtins were
> array_domain_type
> is actually defined with a size of 200.
> The comment that follows that point sounded premonitory :) :)
> 
> /* Make a type for arrays of characters.
>    With luck nothing will ever really depend on the length of this
>    array type.  */
> 
> At least in the current implementation the type is set by
> fix_string_type were the actual string length is taken in account.
> 
> I attach a patch updating the logic accordingly and a new testcase
>  for that.
> 
> make check-jit is passing clean.
> 
> Best Regards
>   Andrea

Sorry about the long delay in reviewing this patch.

> gcc/jit/ChangeLog
> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> 
>         * jit-playback.h
>         (gcc::jit::recording::context m_recording_ctxt): Remove
                     ^^^^^^^^^
		     "playback" here
		     
>         m_char_array_type_node field.

[...]

> @@ -670,9 +669,12 @@ playback::rvalue *
>  playback::context::
>  new_string_literal (const char *value)
>  {
> -  tree t_str = build_string (strlen (value), value);
> -  gcc_assert (m_char_array_type_node);
> -  TREE_TYPE (t_str) = m_char_array_type_node;
> +  /* Compare with c-family/c-common.c: fix_string_type.  */
> +  size_t len = strlen (value);
> +  tree i_type = build_index_type (size_int (len));
> +  tree a_type = build_array_type (char_type_node, i_type);
> +  tree t_str = build_string (len, value);
> +  TREE_TYPE (t_str) = a_type;

This code works with string lengths and string sizes which always
requires a little extra care.  I'd like to see at least a comment
discussing this, as it's not immediately clear to me that we're
correctly handling the NUL terminator here.

Consider the string "foo".
This has strlen == 3, and its size is 4 chars.

build_string's comment says "Note that for a C string literal, LEN
should include the trailing NUL."

However, build_string appears to allocate one byte more than LEN, and
write a NUL in that final position.

fix_string_type has:

  int length = TREE_STRING_LENGTH (value);

where tree.h has:

  /* In C terms, this is sizeof, not strlen.  */
  #define TREE_STRING_LENGTH(NODE) (STRING_CST_CHECK (NODE)-
>string.length)

and:
  nchars = length / charsz;

where for our purposes charsz == 1 and so nchars == length.

fix_string_type has:

  i_type = build_index_type (size_int (nchars - 1));

So I *think* the patch is correct, but there ought to be a comment at
least.

Or maybe use TREE_STRING_LENGTH (t_str) to more closely follow
fix_string_type?

[...]

Also, please add an assertion and comment to the testcase to assert
that the very long string is indeed longer than the previous limit of
200.

Thanks
Dave