From patchwork Thu Mar 24 10:17:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 601509 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3qW2Rv6GsMz9s9Z for ; Thu, 24 Mar 2016 21:17:30 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=VXkjXgcc; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=wS0fOOUERGZO/osd2 bdHoB/F8BXvX2vZ9fwJ/NvtiC663nIU62QNkJOY+FCmAhbqOhOYkTcMLzxGdLoQo xdgjSRNQNtAF1WemTBJHNUZYrwuKz+gjRbRMIQKZwzneXdMrMqWjqz6UEB7MDHt2 CV2LboDBNar7X0+hiNa2eESdwo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=QHfbiX71fVXynfheNgAWMiz Ppa0=; b=VXkjXgcczhJ71wlytN/dP/ctxagQexZ2Y3lrcGpwU8uhXmMq6b80/7r +Kd4FNlZxlYQb+YIuxRfgYBA2OmTLwe2rhrGpHrEn8k3MFXKZqsR0IwYE9zeqqXg MpSZVJy7ZwfU5ZzCGHOIU2+KfO6JV8zfUUnrO8IA98gdRwJdB0Rg= Received: (qmail 28041 invoked by alias); 24 Mar 2016 10:17:21 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 28023 invoked by uid 89); 24 Mar 2016 10:17:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=sk:output_, gcc_assert, helper, meaningless X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 24 Mar 2016 10:17:15 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id BDDBA64D02; Thu, 24 Mar 2016 10:17:13 +0000 (UTC) Received: from reynosa.quesejoda.com (vpn-62-15.rdu2.redhat.com [10.10.62.15]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2OAHAd2011635; Thu, 24 Mar 2016 06:17:11 -0400 Subject: Re: out of bounds access in insn-automata.c To: Bernd Schmidt , GCC Mailing List References: <56F23888.5080506@redhat.com> <56F2B57E.6080300@t-online.de> Cc: "Vladimir N. Makarov" , gcc-patches From: Aldy Hernandez Message-ID: <56F3BEA5.1090007@redhat.com> Date: Thu, 24 Mar 2016 05:17:09 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <56F2B57E.6080300@t-online.de> On 03/23/2016 10:25 AM, Bernd Schmidt wrote: > On 03/23/2016 07:32 AM, Aldy Hernandez wrote: >> >> int >> maximal_insn_latency (rtx insn) >> { >> int insn_code; >> >> if (insn == 0) >> insn_code = DFA__ADVANCE_CYCLE; >> >> >> else >> { >> insn_code = dfa_insn_code (as_a (insn)); >> if (insn_code > DFA__ADVANCE_CYCLE) >> return 0; >> } >> return internal_maximal_insn_latency (insn_code, insn); >> } >> >> In the case where insn==0, insn_code is set to the size of >> default_latencies[] which will get accessed in the return. >> >> Does insn==0 never happen? > > I suspect it never happens in this function. I'd add a gcc_assert to > that effect and try a bootstrap/test. Hmm, it seems to be a sel-sched > only thing so a normal bootstrap would be meaningless, but from the > context it looks fairly clearly like insn is always nonnull. Vlad. Bernd. Thanks for your input. I've added the assert on the caller (maximal_insn_latency), because as you mentioned, the helper function is used for other things in which insn==0 can happen. Now we generate: int maximal_insn_latency (rtx insn) { int insn_code; gcc_assert (insn != 0); // <<<<--- Added code. if (insn == 0) insn_code = DFA__ADVANCE_CYCLE; else { insn_code = dfa_insn_code (as_a (insn)); if (insn_code > DFA__ADVANCE_CYCLE) return 0; } return internal_maximal_insn_latency (insn_code, insn); } > > It looks like this block of code is written by a helper function that is > really intended for other purposes than for maximal_insn_latency. Might > be worth changing to > int insn_code = dfa_insn_code (as_a (insn)); > gcc_assert (insn_code <= DFA__ADVANCE_CYCLE); dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't put that assert on the helper function. While I was at it, I changed the helper function comment to reflect what it has been generating. It was wrong. First round of tests was ok, but test machine died. OK pending tests? Aldy + + * genautomata.c (output_maximal_insn_latency_func): Assert that + insn is non-null. + (output_internal_insn_code_evaluation): Fix comment to match + generated code. diff --git a/gcc/genautomata.c b/gcc/genautomata.c index e3a6c59..91abd2e 100644 --- a/gcc/genautomata.c +++ b/gcc/genautomata.c @@ -8113,14 +8113,14 @@ output_internal_trans_func (void) /* Output code - if (insn != 0) + if (insn == 0) + insn_code = DFA__ADVANCE_CYCLE; + else { insn_code = dfa_insn_code (insn); if (insn_code > DFA__ADVANCE_CYCLE) return code; } - else - insn_code = DFA__ADVANCE_CYCLE; where insn denotes INSN_NAME, insn_code denotes INSN_CODE_NAME, and code denotes CODE. */ @@ -8527,6 +8527,7 @@ output_maximal_insn_latency_func (void) "maximal_insn_latency", INSN_PARAMETER_NAME); fprintf (output_file, "{\n int %s;\n", INTERNAL_INSN_CODE_NAME); + fprintf (output_file, " gcc_assert (%s != 0);\n", INSN_PARAMETER_NAME); output_internal_insn_code_evaluation (INSN_PARAMETER_NAME, INTERNAL_INSN_CODE_NAME, 0); fprintf (output_file, " return %s (%s, %s);\n}\n\n",