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

[Andrea Corallo <Andrea.Corallo@arm.com>]

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