From patchwork Sun Jan 29 19:32:53 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 138474 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]) by ozlabs.org (Postfix) with SMTP id EA756B6EF2 for ; Mon, 30 Jan 2012 06:33:15 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1328470396; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:From:To:Mail-Followup-To:Cc:Subject:Date:Message-ID: User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=wxVnNPxtTezECQXRvDeAskK9lb4=; b=Uhura+ALbSjCsqH jMK62tEvFTe7e5BUtsfxzoB9zNjjnDon15D6SdFiPYacEaZvgq/P6YXLjuJFhian u+TkyiaEcyYBE5yP0xkHPo80Z/ozR6YYR1JqKXuZnqfnudBbGHuJGxvWhuW27xLG ro2VLJYsoAbd8SCyfjCiCOqf6vaY= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:From:To:Mail-Followup-To:Cc:Subject:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=CH4J5OpJ8ydVSOVjsVYBXC1J0YmqaPpgfo+hAdhiaqJBm+Avj3vetankvQctqy pZo8tjx7I6gnLFlspv8ybIC1P1Jq0oCV2CymmvYs5ZjTN2028WxnmhznLHsuwJHW yinsw/wubxwkph4in9Xmlcfm48ygR6TeRHGtlZ9cjJqOU=; Received: (qmail 18659 invoked by alias); 29 Jan 2012 19:33:12 -0000 Received: (qmail 18651 invoked by uid 22791); 29 Jan 2012 19:33:10 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, TW_HF X-Spam-Check-By: sourceware.org Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 29 Jan 2012 19:32:57 +0000 Received: by wibhq7 with SMTP id hq7so3059380wib.20 for ; Sun, 29 Jan 2012 11:32:55 -0800 (PST) Received: by 10.180.103.132 with SMTP id fw4mr13735163wib.3.1327865575770; Sun, 29 Jan 2012 11:32:55 -0800 (PST) Received: from localhost (rsandifo.gotadsl.co.uk. [82.133.89.107]) by mx.google.com with ESMTPS id n3sm44940290wiz.9.2012.01.29.11.32.54 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 Jan 2012 11:32:55 -0800 (PST) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, nickc@redhat.com, rdsandiford@googlemail.com Cc: nickc@redhat.com Subject: PR middle-end/24306 revisited: va_arg and zero-sized objects Date: Sun, 29 Jan 2012 19:32:53 +0000 Message-ID: <877h0a8ftm.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 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 [ Nick, you might remember mentioning this bug a few months back. I think I've finally got a proper fix, rather than the failed attempt I originally sent. ] 2005-12-20 Richard Guenther PR middle-end/24306 * builtins.c (std_gimplify_va_arg_expr): Do not align va frame for zero sized types. * config/i386/i386.c (ix86_gimplify_va_arg): Likewise. made va_arg ignore function_arg_boundary for zero-sized types. I think this was due to the x86_64 definition of function_arg_advance: static void function_arg_advance_64 (CUMULATIVE_ARGS *cum, enum machine_mode mode, const_tree type, HOST_WIDE_INT words, bool named) { int int_nregs, sse_nregs; /* Unnamed 256bit vector mode parameters are passed on stack. */ if (!named && VALID_AVX256_REG_MODE (mode)) return; if (examine_argument (mode, type, 0, &int_nregs, &sse_nregs) && sse_nregs <= cum->sse_nregs && int_nregs <= cum->nregs) { cum->nregs -= int_nregs; cum->sse_nregs -= sse_nregs; cum->regno += int_nregs; cum->sse_regno += sse_nregs; } else { int align = ix86_function_arg_boundary (mode, type) / BITS_PER_WORD; cum->words = (cum->words + align - 1) & ~(align - 1); cum->words += words; } } which does indeed ignore ix86_function_arg_boundary for zero-sized objects within the regparm range. But the target-independent handling of stack arguments doesn't have a similar check, so things go wrong with 6+ arguments. See the testcase below, which fails at count == 6 on x86_64-linux-gnu. I've just realised this is also the cause of a MIPS bug that Nick pointed me at a while ago, and also the cause of the mips-sde-elf struct-layout-1 failures. The MIPS ABIs effectively treat the register parameters as a memory image, and alignment is always honoured: static void mips_get_arg_info (struct mips_arg_info *info, const CUMULATIVE_ARGS *cum, enum machine_mode mode, const_tree type, bool named) { [...] /* See whether the argument has doubleword alignment. */ doubleword_aligned_p = (mips_function_arg_boundary (mode, type) > BITS_PER_WORD); So MIPS wants the original behaviour of always aligning. [ The reason the struct-layout-1 tests didn't fail for mips*-linux-gnu is that the tests were passing a long double after the 16-byte aligned type. n32 and n64 long doubles are usually 16 bytes and usually have 16-byte alignment, so that alignment was saving us. But mips-sde-elf uses a variation of n32 in which long doubles are only 8 bytes. ] The easiest way of fixing this for MIPS without affecting other targets seemed to be to define a std_gimplify_va_arg_expr_always_align function to go alongside std_gimplify_va_arg. This avoids duplicating the whole function in the MIPS backend. The PR trail suggests that this might be a problem for PowerPC too, so maybe other targets would want to use it. Tested on mips64-linux-gnu, mips-sde-elf and x86_64-linux-gnu. OK to install? What about x86_64? The test fails before and after the patch, so should I XFAIL it there for now? I certainly don't know enough about the x86_64 ABI to fix it myself. Thanks, Richard gcc/ PR middle-end/24306 * tree.h (std_gimplify_va_arg_expr_always_align): Declare. * builtins.c (std_gimplify_va_arg_expr_1): New function, split out from... (std_gimplify_va_arg_expr): ...here. (std_gimplify_va_arg_expr_always_align): New function. * config/mips/mips.c (mips_gimplify_va_arg_expr): Call it instead of std_gimplify_va_arg_expr. gcc/testsuite/ PR middle-end/24306 * gcc.dg/va-arg-6.c: New test. Index: gcc/tree.h =================================================================== --- gcc/tree.h 2012-01-29 11:15:28.000000000 +0000 +++ gcc/tree.h 2012-01-29 11:27:25.000000000 +0000 @@ -5442,6 +5442,8 @@ extern tree build_call_expr (tree, int, extern tree mathfn_built_in (tree, enum built_in_function fn); extern tree c_strlen (tree, int); extern tree std_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *); +extern tree std_gimplify_va_arg_expr_always_align (tree, tree, gimple_seq *, + gimple_seq *); extern tree build_va_arg_indirect_ref (tree); extern tree build_string_literal (int, const char *); extern bool validate_arglist (const_tree, ...); Index: gcc/builtins.c =================================================================== --- gcc/builtins.c 2012-01-29 11:15:28.000000000 +0000 +++ gcc/builtins.c 2012-01-29 11:27:25.000000000 +0000 @@ -4229,12 +4229,13 @@ expand_builtin_va_start (tree exp) return const0_rtx; } -/* The "standard" implementation of va_arg: read the value from the - current (padded) address and increment by the (padded) size. */ - -tree -std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p, - gimple_seq *post_p) +/* A subroutine of std_gimplify_va_arg_expr and + std_gimplify_va_arg_expr_always_align, with ALWAYS_ALIGN_P + distinguishing between them. */ + +static tree +std_gimplify_va_arg_expr_1 (tree valist, tree type, gimple_seq *pre_p, + gimple_seq *post_p, bool always_align_p) { tree addr, t, type_size, rounded_size, valist_tmp; unsigned HOST_WIDE_INT align, boundary; @@ -4269,7 +4270,7 @@ std_gimplify_va_arg_expr (tree valist, t /* va_list pointer is aligned to PARM_BOUNDARY. If argument actually requires greater alignment, we must perform dynamic alignment. */ if (boundary > align - && !integer_zerop (TYPE_SIZE (type))) + && (always_align_p || !integer_zerop (TYPE_SIZE (type)))) { t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp, fold_build_pointer_plus_hwi (valist_tmp, boundary - 1)); @@ -4326,6 +4327,26 @@ std_gimplify_va_arg_expr (tree valist, t return build_va_arg_indirect_ref (addr); } +/* The "standard" implementation of va_arg: read the value from the + current (padded) address and increment by the (padded) size. */ + +tree +std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p, + gimple_seq *post_p) +{ + return std_gimplify_va_arg_expr_1 (valist, type, pre_p, post_p, false); +} + +/* Like std_gimplify_va_arg_expr, but honor function_arg_boundary + even for zero-sized containers. */ + +tree +std_gimplify_va_arg_expr_always_align (tree valist, tree type, + gimple_seq *pre_p, gimple_seq *post_p) +{ + return std_gimplify_va_arg_expr_1 (valist, type, pre_p, post_p, true); +} + /* Build an indirect-ref expression over the given TREE, which represents a piece of a va_arg() expansion. */ tree Index: gcc/config/mips/mips.c =================================================================== --- gcc/config/mips/mips.c 2012-01-29 11:15:28.000000000 +0000 +++ gcc/config/mips/mips.c 2012-01-29 11:27:25.000000000 +0000 @@ -5590,7 +5590,7 @@ mips_gimplify_va_arg_expr (tree valist, type = build_pointer_type (type); if (!EABI_FLOAT_VARARGS_P) - addr = std_gimplify_va_arg_expr (valist, type, pre_p, post_p); + addr = std_gimplify_va_arg_expr_always_align (valist, type, pre_p, post_p); else { tree f_ovfl, f_gtop, f_ftop, f_goff, f_foff; Index: gcc/testsuite/gcc.dg/va-arg-6.c =================================================================== --- /dev/null 2012-01-29 09:41:21.178691970 +0000 +++ gcc/testsuite/gcc.dg/va-arg-6.c 2012-01-29 11:36:30.000000000 +0000 @@ -0,0 +1,48 @@ +/* { dg-options "" } */ +/* { dg-do run } */ + +#include + +extern void abort (void); + +struct __attribute__((aligned(16))) empty {}; + +static void __attribute__((noinline)) +check_args (int count, ...) +{ + va_list va; + int i; + + va_start (va, count); + for (i = 0; i < count; i++) + if (va_arg (va, int) != 1000 + i) + abort (); + + va_arg (va, struct empty); + if (va_arg (va, int) != 2000 + count) + abort (); + + va_end (va); +} + +int +main (void) +{ + struct empty e; + + check_args (1, 1000, e, 2001); + check_args (2, 1000, 1001, e, 2002); + check_args (3, 1000, 1001, 1002, e, 2003); + check_args (4, 1000, 1001, 1002, 1003, e, 2004); + check_args (5, 1000, 1001, 1002, 1003, 1004, e, 2005); + check_args (6, 1000, 1001, 1002, 1003, 1004, 1005, e, 2006); + check_args (7, 1000, 1001, 1002, 1003, 1004, 1005, 1006, e, 2007); + check_args (8, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, e, 2008); + check_args (9, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, + 1008, e, 2009); + check_args (10, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, + 1008, 1009, e, 2010); + check_args (11, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, + 1008, 1009, 1010, e, 2011); + return 0; +}