From: Andrea Corallo <Andrea.Corallo@arm.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: "jit@gcc.gnu.org" <jit@gcc.gnu.org>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>, Andrea Corallo <Andrea.Corallo@arm.com>
Subject: Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal
Date: Tue, 01 Jan 2019 00:00:00 -0000 [thread overview]
Message-ID: <gkrftl5zfuq.fsf@arm.com> (raw)
In-Reply-To: <gkrwoernjnj.fsf@arm.com>
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
next prev parent reply other threads:[~2019-09-09 14:47 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-01 0:00 Andrea Corallo
2019-01-01 0:00 ` Andrea Corallo [this message]
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
[not found] <1579772431.117590.ezmlm@gcc.gnu.org>
2020-01-01 0:00 ` [PATCH][gcc] " Andrea Corallo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=gkrftl5zfuq.fsf@arm.com \
--to=andrea.corallo@arm.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).