From patchwork Tue Dec 3 14:12:05 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 296205 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)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 4237E2C0089 for ; Wed, 4 Dec 2013 01:13:25 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=m11wx3EIPJyCNDHfcj x+TA7E7aSoXA4o1fXDOUfb8XVxYc6Q/m6w0rNNKFoTzUUfk2HUOlPBR2aavnUdOm ZPfjLSQ8DgDZ/UygfhywpUNmJMWwYxF1lGHt/JSZ6iKPitHYBka9LsHcplgghTnd WX0gPXwpWvPf2pJRNMI/D5f4Y= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=mKeh26wkheinxrpXEZpyN7fX qmg=; b=cXiu6hsruvIjmpFdTyue7uE/N1xB/VOJsdTaG8+5vMqpl9qbNXJ8iUnq a8sOIxGPb5R2kPvuB0WcKCxTMAC7gNT6YTfUVt7Ez8YZzziV2CZqZZbPqgR3wa8y ngl/ax8U0iEG0SCdchprlOYWW/4Bad6sIJPyeqSjxYuEazk+dyw= Received: (qmail 31986 invoked by alias); 3 Dec 2013 14:13:08 -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 31945 invoked by uid 89); 3 Dec 2013 14:13:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.8 required=5.0 tests=AWL, BAYES_50, FREEMAIL_FROM, RDNS_NONE, SPF_PASS, URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mail-wg0-f50.google.com Received: from Unknown (HELO mail-wg0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 03 Dec 2013 14:12:16 +0000 Received: by mail-wg0-f50.google.com with SMTP id a1so11790427wgh.5 for ; Tue, 03 Dec 2013 06:12:06 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.194.94.167 with SMTP id dd7mr7413634wjb.43.1386079925980; Tue, 03 Dec 2013 06:12:05 -0800 (PST) Received: by 10.195.12.114 with HTTP; Tue, 3 Dec 2013 06:12:05 -0800 (PST) In-Reply-To: References: Date: Tue, 3 Dec 2013 15:12:05 +0100 Message-ID: Subject: Re: [REPOST] Invalid Code when reading from unaligned zero-sized array From: Richard Biener To: Bernd Edlinger Cc: Jeff Law , "gcc-patches@gcc.gnu.org" , Jakub Jelinek X-IsSubscribed: yes On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener wrote: > On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger > wrote: >> Hi Jeff, >> >> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating >> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748 >> >> one test case is this one: >> >> pr57748-3.c: >> /* PR middle-end/57748 */ >> /* { dg-do run } */ >> /* wrong code in expand_expr_real_1. */ >> >> #include >> >> extern void abort (void); >> >> typedef long long V >> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >> >> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1))); >> >> struct __attribute__((packed)) T { char c; P s; }; >> >> void __attribute__((noinline, noclone)) >> check (P *p) >> { >> if (p->b[0][0] != 3 || p->b[0][1] != 4) >> abort (); >> } >> >> void __attribute__((noinline, noclone)) >> foo (struct T *t) >> { >> V a = { 3, 4 }; >> t->s.b[0] = a; >> } >> >> int >> main () >> { >> struct T *t = (struct T *) calloc (128, 1); >> >> foo (t); >> check (&t->s); >> >> free (t); >> return 0; >> } >> >> >> and the other one is >> pr57748-4.c: >> /* PR middle-end/57748 */ >> /* { dg-do run } */ >> /* wrong code in expand_expr_real_1. */ >> >> #include >> >> extern void abort (void); >> >> typedef long long V >> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >> >> typedef struct S { V b[1]; } P __attribute__((aligned (1))); >> >> struct __attribute__((packed)) T { char c; P s; }; >> >> void __attribute__((noinline, noclone)) >> check (P *p) >> { >> if (p->b[1][0] != 3 || p->b[1][1] != 4) >> abort (); >> } >> >> void __attribute__((noinline, noclone)) >> foo (struct T *t) >> { >> V a = { 3, 4 }; >> t->s.b[1] = a; >> } >> >> int >> main () >> { >> struct T *t = (struct T *) calloc (128, 1); >> >> foo (t); >> check (&t->s); >> >> free (t); >> return 0; >> } >> >> >> The patch does add a boolean "expand_reference" parameter to expand_expr_real and >> expand_expr_real_1. I pass true when I intend to use the returned memory context >> as an array reference, instead of a value. At places where mis-aligned values are extracted, >> I do not return a register with the extracted mis-aligned value if expand_reference is true. >> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference" >> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the >> expand_modifier "EXPAND_MEMORY". >> >> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times). >> >> Ok for trunk? > > It still feels like papering over the underlying issue. Let me have a > second (or third?) look. Few comments on your patch. @@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac align = get_object_alignment (exp); if (modifier != EXPAND_WRITE && modifier != EXPAND_MEMORY + && !expand_reference && mode != BLKmode && align < GET_MODE_ALIGNMENT (mode) /* If the target does not have special handling for unaligned (TARGET_MEM_REF), expand_reference should never be true here, there may be no component-refs around TARGET_MEM_REFs. You miss adjusting the VIEW_CONVERT_EXPR path? (line-numbers are off a lot in your patch, context doesn't help very much :/ Does not seem to be against 4.8 either ...) this should use expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); anyway. @@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac op0 = copy_rtx (op0); set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type))); } - else if (mode != BLKmode + else if (modifier != EXPAND_WRITE + && modifier != EXPAND_MEMORY + && !expand_reference + && mode != BLKmode && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode) /* If the target does have special handling for unaligned loads of mode then use them. */ @@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac return reg; } else if (STRICT_ALIGNMENT + && modifier != EXPAND_WRITE + && modifier != EXPAND_MEMORY + && !expand_reference && mode != BLKmode && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)) { why the unrelated change to add the modifier checks? Looks like both if cases are close by and factoring out common code like else if (!expand_reference && mode != BLKmode && MEM_ALIGN (... { if (...) else if (STRICT_ALIGNMENT) would be better, also matching the other similar occurances. Looking for some more time your patch may be indeed the easiest without big re-factoring. Richard. > Richard. > >> >> Thanks >> Bernd. Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 204411) +++ gcc/cfgexpand.c (working copy) @@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt) if (lhs) expand_assignment (lhs, exp, false); else - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL); + expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false); mark_transaction_restart_calls (stmt); }