From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85907 invoked by alias); 6 Mar 2020 03:18:56 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 85836 invoked by uid 89); 6 Mar 2020 03:18:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=unavailable version=3.3.1 spammy= X-Spam-Status: No, score=-10.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Mar 2020 03:18:55 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583464733; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=djoxEEjQy/lAr3ou5DLS70EmnpHNbhiCib210Nxe0Nk=; b=a/CRsGtT+vxPIm6bYavunXdspzEUjOFiLki32Ihu2dcbMlJ5+O7xAdU3NG5s6LRal7BKqg W12WXV2A8pUlgDpjenkLoWgZRYc55y+DGTaeG6+0wVpNHBT7hyPP6ui7EeB3SyP0Km/elA 5AojtSYSm+vvENbNf45BJPSkNtSppDc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-388-1a178_4fNnKtuJAwVCAxgw-1; Thu, 05 Mar 2020 22:18:49 -0500 X-MC-Unique: 1a178_4fNnKtuJAwVCAxgw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 79D94107ACCA; Fri, 6 Mar 2020 03:18:48 +0000 (UTC) Received: from ovpn-116-56.phx2.redhat.com (ovpn-116-56.phx2.redhat.com [10.3.116.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE2B99A6A; Fri, 6 Mar 2020 03:18:47 +0000 (UTC) Message-ID: Subject: Re: [PATCH][gcc] libgccjit: handle long literals in playback::context::new_string_literal From: David Malcolm To: Andrea Corallo , "jit@gcc.gnu.org" , "gcc-patches@gcc.gnu.org" Cc: nd Date: Wed, 01 Jan 2020 00:00:00 -0000 In-Reply-To: References: User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-q1/txt/msg00008.txt 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 > > * 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