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

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

[Andrea Corallo]

> Andrea Corallo writes:
>
>> Andrea Corallo writes:
>>
>>> Andrea Corallo writes:
>>>
>>>> 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
>>>>
>>>> gcc/jit/ChangeLog
>>>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>>>
>>>> 	* jit-playback.h
>>>> 	(gcc::jit::recording::context m_recording_ctxt): Remove
>>>> 	m_char_array_type_node field.
>>>> 	* jit-playback.c
>>>> 	(playback::context::context) Remove m_char_array_type_node from member
>>>> 	initializer list.
>>>> 	(playback::context::new_string_literal) Fix logic to handle string
>>>> 	length > 200.
>>>>
>>>> gcc/testsuite/ChangeLog
>>>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>>>
>>>> 	* jit.dg/all-non-failing-tests.h: Add test-long-string-literal.c.
>>>> 	* jit.dg/test-long-string-literal.c: New testcase.
>>>> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
>>>> index 9eeb2a7..a26b8d3 100644
>>>> --- a/gcc/jit/jit-playback.c
>>>> +++ b/gcc/jit/jit-playback.c
>>>> @@ -88,7 +88,6 @@ playback::context::context (recording::context *ctxt)
>>>>    : log_user (ctxt->get_logger ()),
>>>>      m_recording_ctxt (ctxt),
>>>>      m_tempdir (NULL),
>>>> -    m_char_array_type_node (NULL),
>>>>      m_const_char_ptr (NULL)
>>>>  {
>>>>    JIT_LOG_SCOPE (get_logger ());
>>>> @@ -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;
>>>>
>>>>    /* Convert to (const char*), loosely based on
>>>>       c/c-typeck.c: array_to_pointer_conversion,
>>>> @@ -2703,10 +2705,6 @@ playback::context::
>>>>  replay ()
>>>>  {
>>>>    JIT_LOG_SCOPE (get_logger ());
>>>> -  /* Adapted from c-common.c:c_common_nodes_and_builtins.  */
>>>> -  tree array_domain_type = build_index_type (size_int (200));
>>>> -  m_char_array_type_node
>>>> -    = build_array_type (char_type_node, array_domain_type);
>>>>
>>>>    m_const_char_ptr
>>>>      = build_pointer_type (build_qualified_type (char_type_node,
>>>> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
>>>> index d4b148e..801f610 100644
>>>> --- a/gcc/jit/jit-playback.h
>>>> +++ b/gcc/jit/jit-playback.h
>>>> @@ -322,7 +322,6 @@ private:
>>>>
>>>>    auto_vec<function *> m_functions;
>>>>    auto_vec<tree> m_globals;
>>>> -  tree m_char_array_type_node;
>>>>    tree m_const_char_ptr;
>>>>
>>>>    /* Source location handling.  */
>>>> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>>> index 0272e6f8..1b3d561 100644
>>>> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>>> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>>> @@ -220,6 +220,13 @@
>>>>  #undef create_code
>>>>  #undef verify_code
>>>>
>>>> +/* test-long-string-literal.c */
>>>> +#define create_code create_code_long_string_literal
>>>> +#define verify_code verify_code_long_string_literal
>>>> +#include "test-long-string-literal.c"
>>>> +#undef create_code
>>>> +#undef verify_code
>>>> +
>>>>  /* test-sum-of-squares.c */
>>>>  #define create_code create_code_sum_of_squares
>>>>  #define verify_code verify_code_sum_of_squares
>>>> diff --git a/gcc/testsuite/jit.dg/test-long-string-literal.c b/gcc/testsuite/jit.dg/test-long-string-literal.c
>>>> new file mode 100644
>>>> index 0000000..882567c
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
>>>> @@ -0,0 +1,48 @@
>>>> +#include <stdlib.h>
>>>> +#include <stdio.h>
>>>> +#include <string.h>
>>>> +
>>>> +#include "libgccjit.h"
>>>> +
>>>> +#include "harness.h"
>>>> +
>>>> +const char very_long_string[] =
>>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>>> +  "abcabcabcabcabcabcabcabcabcabca";
>>>> +
>>>> +void
>>>> +create_code (gcc_jit_context *ctxt, void *user_data)
>>>> +{
>>>> +  /* Build the test_fn.  */
>>>> +  gcc_jit_function *f =
>>>> +    gcc_jit_context_new_function (
>>>> +      ctxt, NULL,
>>>> +      GCC_JIT_FUNCTION_EXPORTED,
>>>> +      gcc_jit_context_get_type(ctxt,
>>>> +			       GCC_JIT_TYPE_CONST_CHAR_PTR),
>>>> +				"test_long_string_literal",
>>>> +				0, NULL, 0);
>>>> +  gcc_jit_block *blk =
>>>> +    gcc_jit_function_new_block (f, "init_block");
>>>> +  gcc_jit_rvalue *res =
>>>> +    gcc_jit_context_new_string_literal (ctxt, very_long_string);
>>>> +
>>>> +  gcc_jit_block_end_with_return (blk, NULL, res);
>>>> +}
>>>> +
>>>> +void
>>>> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
>>>> +{
>>>> +  typedef const char *(*fn_type) (void);
>>>> +  CHECK_NON_NULL (result);
>>>> +  fn_type test_long_string_literal =
>>>> +    (fn_type)gcc_jit_result_get_code (result, "test_long_string_literal");
>>>> +  CHECK_NON_NULL (test_long_string_literal);
>>>> +
>>>> +  /* Call the JIT-generated function.  */
>>>> +  const char *str = test_long_string_literal ();
>>>> +  CHECK_NON_NULL (str);
>>>> +  CHECK_VALUE (strcmp (str, very_long_string), 0);
>>>> +}
>>>
>>> Kindly pinging
>>>
>>> Bests
>>>   Andrea
>>
>> Hi,
>> I'd like to ping this patch.
>>
>> Bests
>>  Andrea
>
> Pinging again.
>
> Bests
>   Andrea
> ----------

Ping

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

[Andrea Corallo]

David Malcolm <dmalcolm@redhat.com> writes:

> 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?
>

Mmmm when I wrote the patch I looked into the build_string code missing
the comment and that was my understanding too.  But looking better now
in the compiler I think the patch is *not* correct.  The trouble is that
if we call build_string (3, "foo") (as we would do now) we set
TREE_STRING_LENGTH to 3 and this appears to be wrong.

Actually build_string can be called using the strlen as argument (see
for example the various definition of DEF_ATTR_STRING) but apparently
this is not how is done for C strings in the frontends.  Given that this
is what the comment suggests and that modifying the patch with the +1
keeps it functional I think doing what the front-end does is the right
thing.

> 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.

Ack

> Thanks
> Dave

Thanks for reviewing

  Andrea

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

[David Malcolm]

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

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

[Andrea Corallo]

Andrea Corallo writes:

> 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
>
> gcc/jit/ChangeLog
> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>
> 	* jit-playback.h
> 	(gcc::jit::recording::context m_recording_ctxt): Remove
> 	m_char_array_type_node field.
> 	* jit-playback.c
> 	(playback::context::context) Remove m_char_array_type_node from member
> 	initializer list.
> 	(playback::context::new_string_literal) Fix logic to handle string
> 	length > 200.
>
> gcc/testsuite/ChangeLog
> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>
> 	* jit.dg/all-non-failing-tests.h: Add test-long-string-literal.c.
> 	* jit.dg/test-long-string-literal.c: New testcase.
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index 9eeb2a7..a26b8d3 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -88,7 +88,6 @@ playback::context::context (recording::context *ctxt)
>    : log_user (ctxt->get_logger ()),
>      m_recording_ctxt (ctxt),
>      m_tempdir (NULL),
> -    m_char_array_type_node (NULL),
>      m_const_char_ptr (NULL)
>  {
>    JIT_LOG_SCOPE (get_logger ());
> @@ -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;
>
>    /* Convert to (const char*), loosely based on
>       c/c-typeck.c: array_to_pointer_conversion,
> @@ -2703,10 +2705,6 @@ playback::context::
>  replay ()
>  {
>    JIT_LOG_SCOPE (get_logger ());
> -  /* Adapted from c-common.c:c_common_nodes_and_builtins.  */
> -  tree array_domain_type = build_index_type (size_int (200));
> -  m_char_array_type_node
> -    = build_array_type (char_type_node, array_domain_type);
>
>    m_const_char_ptr
>      = build_pointer_type (build_qualified_type (char_type_node,
> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> index d4b148e..801f610 100644
> --- a/gcc/jit/jit-playback.h
> +++ b/gcc/jit/jit-playback.h
> @@ -322,7 +322,6 @@ private:
>
>    auto_vec<function *> m_functions;
>    auto_vec<tree> m_globals;
> -  tree m_char_array_type_node;
>    tree m_const_char_ptr;
>
>    /* Source location handling.  */
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 0272e6f8..1b3d561 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -220,6 +220,13 @@
>  #undef create_code
>  #undef verify_code
>
> +/* test-long-string-literal.c */
> +#define create_code create_code_long_string_literal
> +#define verify_code verify_code_long_string_literal
> +#include "test-long-string-literal.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-sum-of-squares.c */
>  #define create_code create_code_sum_of_squares
>  #define verify_code verify_code_sum_of_squares
> diff --git a/gcc/testsuite/jit.dg/test-long-string-literal.c b/gcc/testsuite/jit.dg/test-long-string-literal.c
> new file mode 100644
> index 0000000..882567c
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
> @@ -0,0 +1,48 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +const char very_long_string[] =
> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
> +  "abcabcabcabcabcabcabcabcabcabca";
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  /* Build the test_fn.  */
> +  gcc_jit_function *f =
> +    gcc_jit_context_new_function (
> +      ctxt, NULL,
> +      GCC_JIT_FUNCTION_EXPORTED,
> +      gcc_jit_context_get_type(ctxt,
> +			       GCC_JIT_TYPE_CONST_CHAR_PTR),
> +				"test_long_string_literal",
> +				0, NULL, 0);
> +  gcc_jit_block *blk =
> +    gcc_jit_function_new_block (f, "init_block");
> +  gcc_jit_rvalue *res =
> +    gcc_jit_context_new_string_literal (ctxt, very_long_string);
> +
> +  gcc_jit_block_end_with_return (blk, NULL, res);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  typedef const char *(*fn_type) (void);
> +  CHECK_NON_NULL (result);
> +  fn_type test_long_string_literal =
> +    (fn_type)gcc_jit_result_get_code (result, "test_long_string_literal");
> +  CHECK_NON_NULL (test_long_string_literal);
> +
> +  /* Call the JIT-generated function.  */
> +  const char *str = test_long_string_literal ();
> +  CHECK_NON_NULL (str);
> +  CHECK_VALUE (strcmp (str, very_long_string), 0);
> +}

Kindly pinging

Bests
  Andrea

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

[Andrea Corallo]

Andrea Corallo writes:

> Andrea Corallo writes:
>
>> 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
>>
>> gcc/jit/ChangeLog
>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>
>> 	* jit-playback.h
>> 	(gcc::jit::recording::context m_recording_ctxt): Remove
>> 	m_char_array_type_node field.
>> 	* jit-playback.c
>> 	(playback::context::context) Remove m_char_array_type_node from member
>> 	initializer list.
>> 	(playback::context::new_string_literal) Fix logic to handle string
>> 	length > 200.
>>
>> gcc/testsuite/ChangeLog
>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>
>> 	* jit.dg/all-non-failing-tests.h: Add test-long-string-literal.c.
>> 	* jit.dg/test-long-string-literal.c: New testcase.
>> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
>> index 9eeb2a7..a26b8d3 100644
>> --- a/gcc/jit/jit-playback.c
>> +++ b/gcc/jit/jit-playback.c
>> @@ -88,7 +88,6 @@ playback::context::context (recording::context *ctxt)
>>    : log_user (ctxt->get_logger ()),
>>      m_recording_ctxt (ctxt),
>>      m_tempdir (NULL),
>> -    m_char_array_type_node (NULL),
>>      m_const_char_ptr (NULL)
>>  {
>>    JIT_LOG_SCOPE (get_logger ());
>> @@ -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;
>>
>>    /* Convert to (const char*), loosely based on
>>       c/c-typeck.c: array_to_pointer_conversion,
>> @@ -2703,10 +2705,6 @@ playback::context::
>>  replay ()
>>  {
>>    JIT_LOG_SCOPE (get_logger ());
>> -  /* Adapted from c-common.c:c_common_nodes_and_builtins.  */
>> -  tree array_domain_type = build_index_type (size_int (200));
>> -  m_char_array_type_node
>> -    = build_array_type (char_type_node, array_domain_type);
>>
>>    m_const_char_ptr
>>      = build_pointer_type (build_qualified_type (char_type_node,
>> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
>> index d4b148e..801f610 100644
>> --- a/gcc/jit/jit-playback.h
>> +++ b/gcc/jit/jit-playback.h
>> @@ -322,7 +322,6 @@ private:
>>
>>    auto_vec<function *> m_functions;
>>    auto_vec<tree> m_globals;
>> -  tree m_char_array_type_node;
>>    tree m_const_char_ptr;
>>
>>    /* Source location handling.  */
>> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>> index 0272e6f8..1b3d561 100644
>> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
>> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>> @@ -220,6 +220,13 @@
>>  #undef create_code
>>  #undef verify_code
>>
>> +/* test-long-string-literal.c */
>> +#define create_code create_code_long_string_literal
>> +#define verify_code verify_code_long_string_literal
>> +#include "test-long-string-literal.c"
>> +#undef create_code
>> +#undef verify_code
>> +
>>  /* test-sum-of-squares.c */
>>  #define create_code create_code_sum_of_squares
>>  #define verify_code verify_code_sum_of_squares
>> diff --git a/gcc/testsuite/jit.dg/test-long-string-literal.c b/gcc/testsuite/jit.dg/test-long-string-literal.c
>> new file mode 100644
>> index 0000000..882567c
>> --- /dev/null
>> +++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
>> @@ -0,0 +1,48 @@
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +#include "libgccjit.h"
>> +
>> +#include "harness.h"
>> +
>> +const char very_long_string[] =
>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>> +  "abcabcabcabcabcabcabcabcabcabca";
>> +
>> +void
>> +create_code (gcc_jit_context *ctxt, void *user_data)
>> +{
>> +  /* Build the test_fn.  */
>> +  gcc_jit_function *f =
>> +    gcc_jit_context_new_function (
>> +      ctxt, NULL,
>> +      GCC_JIT_FUNCTION_EXPORTED,
>> +      gcc_jit_context_get_type(ctxt,
>> +			       GCC_JIT_TYPE_CONST_CHAR_PTR),
>> +				"test_long_string_literal",
>> +				0, NULL, 0);
>> +  gcc_jit_block *blk =
>> +    gcc_jit_function_new_block (f, "init_block");
>> +  gcc_jit_rvalue *res =
>> +    gcc_jit_context_new_string_literal (ctxt, very_long_string);
>> +
>> +  gcc_jit_block_end_with_return (blk, NULL, res);
>> +}
>> +
>> +void
>> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
>> +{
>> +  typedef const char *(*fn_type) (void);
>> +  CHECK_NON_NULL (result);
>> +  fn_type test_long_string_literal =
>> +    (fn_type)gcc_jit_result_get_code (result, "test_long_string_literal");
>> +  CHECK_NON_NULL (test_long_string_literal);
>> +
>> +  /* Call the JIT-generated function.  */
>> +  const char *str = test_long_string_literal ();
>> +  CHECK_NON_NULL (str);
>> +  CHECK_VALUE (strcmp (str, very_long_string), 0);
>> +}
>
> Kindly pinging
>
> Bests
>   Andrea

Hi,
I'd like to ping this patch.

Bests
 Andrea

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

[Andrea Corallo]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

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

gcc/jit/ChangeLog
2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* jit-playback.h
	(gcc::jit::recording::context m_recording_ctxt): Remove
	m_char_array_type_node field.
	* jit-playback.c
	(playback::context::context) Remove m_char_array_type_node from member
	initializer list.
	(playback::context::new_string_literal) Fix logic to handle string
	length > 200.

gcc/testsuite/ChangeLog
2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* jit.dg/all-non-failing-tests.h: Add test-long-string-literal.c.
	* jit.dg/test-long-string-literal.c: New testcase.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: str.patch --]
[-- Type: text/x-diff; name="str.patch", Size: 4245 bytes --]

diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 9eeb2a7..a26b8d3 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -88,7 +88,6 @@ playback::context::context (recording::context *ctxt)
   : log_user (ctxt->get_logger ()),
     m_recording_ctxt (ctxt),
     m_tempdir (NULL),
-    m_char_array_type_node (NULL),
     m_const_char_ptr (NULL)
 {
   JIT_LOG_SCOPE (get_logger ());
@@ -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;
 
   /* Convert to (const char*), loosely based on
      c/c-typeck.c: array_to_pointer_conversion,
@@ -2703,10 +2705,6 @@ playback::context::
 replay ()
 {
   JIT_LOG_SCOPE (get_logger ());
-  /* Adapted from c-common.c:c_common_nodes_and_builtins.  */
-  tree array_domain_type = build_index_type (size_int (200));
-  m_char_array_type_node
-    = build_array_type (char_type_node, array_domain_type);
 
   m_const_char_ptr
     = build_pointer_type (build_qualified_type (char_type_node,
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index d4b148e..801f610 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -322,7 +322,6 @@ private:
 
   auto_vec<function *> m_functions;
   auto_vec<tree> m_globals;
-  tree m_char_array_type_node;
   tree m_const_char_ptr;
 
   /* Source location handling.  */
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f8..1b3d561 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -220,6 +220,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-long-string-literal.c */
+#define create_code create_code_long_string_literal
+#define verify_code verify_code_long_string_literal
+#include "test-long-string-literal.c"
+#undef create_code
+#undef verify_code
+
 /* test-sum-of-squares.c */
 #define create_code create_code_sum_of_squares
 #define verify_code verify_code_sum_of_squares
diff --git a/gcc/testsuite/jit.dg/test-long-string-literal.c b/gcc/testsuite/jit.dg/test-long-string-literal.c
new file mode 100644
index 0000000..882567c
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
@@ -0,0 +1,48 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+const char very_long_string[] =
+  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
+  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
+  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
+  "abcabcabcabcabcabcabcabcabcabca";
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Build the test_fn.  */
+  gcc_jit_function *f =
+    gcc_jit_context_new_function (
+      ctxt, NULL,
+      GCC_JIT_FUNCTION_EXPORTED,
+      gcc_jit_context_get_type(ctxt,
+			       GCC_JIT_TYPE_CONST_CHAR_PTR),
+				"test_long_string_literal",
+				0, NULL, 0);
+  gcc_jit_block *blk =
+    gcc_jit_function_new_block (f, "init_block");
+  gcc_jit_rvalue *res =
+    gcc_jit_context_new_string_literal (ctxt, very_long_string);
+
+  gcc_jit_block_end_with_return (blk, NULL, res);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  typedef const char *(*fn_type) (void);
+  CHECK_NON_NULL (result);
+  fn_type test_long_string_literal =
+    (fn_type)gcc_jit_result_get_code (result, "test_long_string_literal");
+  CHECK_NON_NULL (test_long_string_literal);
+
+  /* Call the JIT-generated function.  */
+  const char *str = test_long_string_literal ();
+  CHECK_NON_NULL (str);
+  CHECK_VALUE (strcmp (str, very_long_string), 0);
+}

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

[Andrea Corallo]

Andrea Corallo writes:

> Andrea Corallo writes:
>
>> Andrea Corallo writes:
>>
>>> 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
>>>
>>> gcc/jit/ChangeLog
>>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>>
>>> 	* jit-playback.h
>>> 	(gcc::jit::recording::context m_recording_ctxt): Remove
>>> 	m_char_array_type_node field.
>>> 	* jit-playback.c
>>> 	(playback::context::context) Remove m_char_array_type_node from member
>>> 	initializer list.
>>> 	(playback::context::new_string_literal) Fix logic to handle string
>>> 	length > 200.
>>>
>>> gcc/testsuite/ChangeLog
>>> 2019-??-??  Andrea Corallo  <andrea.corallo@arm.com>
>>>
>>> 	* jit.dg/all-non-failing-tests.h: Add test-long-string-literal.c.
>>> 	* jit.dg/test-long-string-literal.c: New testcase.
>>> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
>>> index 9eeb2a7..a26b8d3 100644
>>> --- a/gcc/jit/jit-playback.c
>>> +++ b/gcc/jit/jit-playback.c
>>> @@ -88,7 +88,6 @@ playback::context::context (recording::context *ctxt)
>>>    : log_user (ctxt->get_logger ()),
>>>      m_recording_ctxt (ctxt),
>>>      m_tempdir (NULL),
>>> -    m_char_array_type_node (NULL),
>>>      m_const_char_ptr (NULL)
>>>  {
>>>    JIT_LOG_SCOPE (get_logger ());
>>> @@ -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;
>>>
>>>    /* Convert to (const char*), loosely based on
>>>       c/c-typeck.c: array_to_pointer_conversion,
>>> @@ -2703,10 +2705,6 @@ playback::context::
>>>  replay ()
>>>  {
>>>    JIT_LOG_SCOPE (get_logger ());
>>> -  /* Adapted from c-common.c:c_common_nodes_and_builtins.  */
>>> -  tree array_domain_type = build_index_type (size_int (200));
>>> -  m_char_array_type_node
>>> -    = build_array_type (char_type_node, array_domain_type);
>>>
>>>    m_const_char_ptr
>>>      = build_pointer_type (build_qualified_type (char_type_node,
>>> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
>>> index d4b148e..801f610 100644
>>> --- a/gcc/jit/jit-playback.h
>>> +++ b/gcc/jit/jit-playback.h
>>> @@ -322,7 +322,6 @@ private:
>>>
>>>    auto_vec<function *> m_functions;
>>>    auto_vec<tree> m_globals;
>>> -  tree m_char_array_type_node;
>>>    tree m_const_char_ptr;
>>>
>>>    /* Source location handling.  */
>>> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>> index 0272e6f8..1b3d561 100644
>>> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>>> @@ -220,6 +220,13 @@
>>>  #undef create_code
>>>  #undef verify_code
>>>
>>> +/* test-long-string-literal.c */
>>> +#define create_code create_code_long_string_literal
>>> +#define verify_code verify_code_long_string_literal
>>> +#include "test-long-string-literal.c"
>>> +#undef create_code
>>> +#undef verify_code
>>> +
>>>  /* test-sum-of-squares.c */
>>>  #define create_code create_code_sum_of_squares
>>>  #define verify_code verify_code_sum_of_squares
>>> diff --git a/gcc/testsuite/jit.dg/test-long-string-literal.c b/gcc/testsuite/jit.dg/test-long-string-literal.c
>>> new file mode 100644
>>> index 0000000..882567c
>>> --- /dev/null
>>> +++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
>>> @@ -0,0 +1,48 @@
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +
>>> +#include "libgccjit.h"
>>> +
>>> +#include "harness.h"
>>> +
>>> +const char very_long_string[] =
>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>> +  "abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc"
>>> +  "abcabcabcabcabcabcabcabcabcabca";
>>> +
>>> +void
>>> +create_code (gcc_jit_context *ctxt, void *user_data)
>>> +{
>>> +  /* Build the test_fn.  */
>>> +  gcc_jit_function *f =
>>> +    gcc_jit_context_new_function (
>>> +      ctxt, NULL,
>>> +      GCC_JIT_FUNCTION_EXPORTED,
>>> +      gcc_jit_context_get_type(ctxt,
>>> +			       GCC_JIT_TYPE_CONST_CHAR_PTR),
>>> +				"test_long_string_literal",
>>> +				0, NULL, 0);
>>> +  gcc_jit_block *blk =
>>> +    gcc_jit_function_new_block (f, "init_block");
>>> +  gcc_jit_rvalue *res =
>>> +    gcc_jit_context_new_string_literal (ctxt, very_long_string);
>>> +
>>> +  gcc_jit_block_end_with_return (blk, NULL, res);
>>> +}
>>> +
>>> +void
>>> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
>>> +{
>>> +  typedef const char *(*fn_type) (void);
>>> +  CHECK_NON_NULL (result);
>>> +  fn_type test_long_string_literal =
>>> +    (fn_type)gcc_jit_result_get_code (result, "test_long_string_literal");
>>> +  CHECK_NON_NULL (test_long_string_literal);
>>> +
>>> +  /* Call the JIT-generated function.  */
>>> +  const char *str = test_long_string_literal ();
>>> +  CHECK_NON_NULL (str);
>>> +  CHECK_VALUE (strcmp (str, very_long_string), 0);
>>> +}
>>
>> Kindly pinging
>>
>> Bests
>>   Andrea
>
> Hi,
> I'd like to ping this patch.
>
> Bests
>  Andrea

Pinging again.

Bests
  Andrea