* Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2019-01-01 0:00 [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal Andrea Corallo
@ 2019-01-01 0:00 ` Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
2020-01-01 0:00 ` David Malcolm
1 sibling, 1 reply; 10+ messages in thread
From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, gcc-patches, nd, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2019-01-01 0:00 ` Andrea Corallo
@ 2019-01-01 0:00 ` Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
0 siblings, 1 reply; 10+ messages in thread
From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: Andrea Corallo, jit, gcc-patches, nd
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
@ 2019-01-01 0:00 Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
2020-01-01 0:00 ` David Malcolm
0 siblings, 2 replies; 10+ messages in thread
From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw)
To: jit, gcc-patches; +Cc: nd
[-- 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);
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2019-01-01 0:00 ` Andrea Corallo
@ 2019-01-01 0:00 ` Andrea Corallo
0 siblings, 0 replies; 10+ messages in thread
From: Andrea Corallo @ 2019-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: Andrea Corallo, jit, gcc-patches, nd
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2019-01-01 0:00 [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
@ 2020-01-01 0:00 ` David Malcolm
2020-03-07 21:43 ` Andrea Corallo
1 sibling, 1 reply; 10+ messages in thread
From: David Malcolm @ 2020-01-01 0:00 UTC (permalink / raw)
To: Andrea Corallo, jit, gcc-patches; +Cc: nd
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2020-01-01 0:00 ` David Malcolm
@ 2020-03-07 21:43 ` Andrea Corallo
2020-03-09 22:20 ` [PATCH v2][gcc] " Andrea Corallo
0 siblings, 1 reply; 10+ messages in thread
From: Andrea Corallo @ 2020-03-07 21:43 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, gcc-patches, nd
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2020-03-07 21:43 ` Andrea Corallo
@ 2020-03-09 22:20 ` Andrea Corallo
2020-03-18 22:52 ` Andrea Corallo
2020-03-21 1:41 ` David Malcolm
0 siblings, 2 replies; 10+ messages in thread
From: Andrea Corallo @ 2020-03-09 22:20 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, nd, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 733 bytes --]
Hi all,
second version of the patch for the 200 characters limit for literal
strings addressing comments.
make check-jit is passing clean.
Best Regards
Andrea
gcc/jit/ChangeLog
2020-??-?? Andrea Corallo <andrea.corallo@arm.com>
* jit-playback.h
(gcc::jit::playback::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
2020-??-?? 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, Size: 4454 bytes --]
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 904cc167a0c..074434a9f6b 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/jit/jit-playback.c b/gcc/jit/jit-playback.c
index da687002a72..d2c8bb4c154 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,14 @@ 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);
+ /* build_string len parameter must include NUL terminator when
+ building C strings. */
+ tree t_str = build_string (len + 1, value);
+ TREE_TYPE (t_str) = a_type;
/* Convert to (const char*), loosely based on
c/c-typeck.c: array_to_pointer_conversion,
@@ -2701,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/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f846f..1b3d5618779 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 00000000000..6caaa781c0b
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
@@ -0,0 +1,54 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.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");
+
+ /* very_long_string is longer than 200 characters to specifically
+ check that the previous limitation no longer apply. */
+
+ assert (sizeof (very_long_string) > 200);
+ 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);
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2020-03-09 22:20 ` [PATCH v2][gcc] " Andrea Corallo
@ 2020-03-18 22:52 ` Andrea Corallo
2020-03-21 1:41 ` David Malcolm
1 sibling, 0 replies; 10+ messages in thread
From: Andrea Corallo @ 2020-03-18 22:52 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, nd, gcc-patches
Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi all,
>
> second version of the patch for the 200 characters limit for literal
> strings addressing comments.
>
> make check-jit is passing clean.
>
> Best Regards
> Andrea
>
> gcc/jit/ChangeLog
> 2020-??-?? Andrea Corallo <andrea.corallo@arm.com>
>
> * jit-playback.h
> (gcc::jit::playback::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
> 2020-??-?? 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.
Kind ping, okay for trunk?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2020-03-09 22:20 ` [PATCH v2][gcc] " Andrea Corallo
2020-03-18 22:52 ` Andrea Corallo
@ 2020-03-21 1:41 ` David Malcolm
2020-03-23 18:04 ` [committed][gcc] " Andrea Corallo
1 sibling, 1 reply; 10+ messages in thread
From: David Malcolm @ 2020-03-21 1:41 UTC (permalink / raw)
To: Andrea Corallo; +Cc: jit, nd, gcc-patches
On Mon, 2020-03-09 at 22:20 +0000, Andrea Corallo wrote:
> Hi all,
>
> second version of the patch for the 200 characters limit for literal
> strings addressing comments.
>
> make check-jit is passing clean.
>
> Best Regards
> Andrea
>
> gcc/jit/ChangeLog
> 2020-??-?? Andrea Corallo <andrea.corallo@arm.com>
>
> * jit-playback.h
> (gcc::jit::playback::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
> 2020-??-?? 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/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f846f..1b3d5618779 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
+
Please adjust where you add this so that these things are in
alphabetical order.
An entry also needs to be added to the "testcases" array at the end of
the header for this to do anything.
OK with that change.
David
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [committed][gcc] libgccjit: handle long literals in playback::context::new_string_literal
2020-03-21 1:41 ` David Malcolm
@ 2020-03-23 18:04 ` Andrea Corallo
0 siblings, 0 replies; 10+ messages in thread
From: Andrea Corallo @ 2020-03-23 18:04 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, nd, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 837 bytes --]
Hi all,
attached the final version of the patch for the 200 characters limit
for literal strings addressing comments on the missing "testcases"
array entry (apologies) and alphabetic order.
make check-jit is passing clean.
Pushed into trunk.
Best Regards
Andrea
gcc/jit/ChangeLog
2020-03-23 Andrea Corallo <andrea.corallo@arm.com>
* jit-playback.h
(gcc::jit::playback::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
2020-03-23 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, Size: 4746 bytes --]
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 904cc167a0c..074434a9f6b 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/jit/jit-playback.c b/gcc/jit/jit-playback.c
index da687002a72..d2c8bb4c154 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,14 @@ 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);
+ /* build_string len parameter must include NUL terminator when
+ building C strings. */
+ tree t_str = build_string (len + 1, value);
+ TREE_TYPE (t_str) = a_type;
/* Convert to (const char*), loosely based on
c/c-typeck.c: array_to_pointer_conversion,
@@ -2701,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/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 0272e6f846f..b2acc74ae95 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -178,6 +178,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-quadratic.c */
#define create_code create_code_quadratic
#define verify_code verify_code_quadratic
@@ -342,6 +349,9 @@ const struct testcase testcases[] = {
{"long_names",
create_code_long_names,
verify_code_long_names},
+ {"long_string_literal",
+ create_code_long_string_literal,
+ verify_code_long_string_literal},
{"quadratic",
create_code_quadratic,
verify_code_quadratic},
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 00000000000..6caaa781c0b
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-long-string-literal.c
@@ -0,0 +1,54 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.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");
+
+ /* very_long_string is longer than 200 characters to specifically
+ check that the previous limitation no longer apply. */
+
+ assert (sizeof (very_long_string) > 200);
+ 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);
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-23 18:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-01 0:00 [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo
2020-01-01 0:00 ` David Malcolm
2020-03-07 21:43 ` Andrea Corallo
2020-03-09 22:20 ` [PATCH v2][gcc] " Andrea Corallo
2020-03-18 22:52 ` Andrea Corallo
2020-03-21 1:41 ` David Malcolm
2020-03-23 18:04 ` [committed][gcc] " Andrea Corallo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).