From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61788 invoked by alias); 28 Dec 2017 20:37:19 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 61772 invoked by uid 89); 28 Dec 2017 20:37:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=tended, disregard, throwing X-HELO: mail-wr0-f171.google.com Received: from mail-wr0-f171.google.com (HELO mail-wr0-f171.google.com) (209.85.128.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Dec 2017 20:37:17 +0000 Received: by mail-wr0-f171.google.com with SMTP id f8so29365731wre.4 for ; Thu, 28 Dec 2017 12:37:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:mail-followup-to:cc:subject:references :date:in-reply-to:message-id:user-agent:mime-version; bh=hCgv0/yDwinO4alh/rZRWd7TE81bGBWUL5QowUYezwA=; b=l5UaJdEsJDChyUf1T1LdhKakFQgrhtG4rJiB4wfh+KSg540i/X8vNgLHLBCGdOd+8c fZVxs36pn68uS+50P+9Krvms9fXI1ylAcyKVUkBZ48gZxk13zFhO8ItSJFxVcid5Uij7 vfukKG8mp/Gys4CgNcAat0bOwdG+iXWI/4HXJgXAd0v/aE1oqBAoQr8pwZvawYlhG/Kl f1yBtRic0UwrpOy3tVivWKIk9baVBp3nPWTQPHhQRh7fxyBSTc1iiqzPUflDQaRt8XPd /e0V+al1e8CiBfzEckwP+58Ed/jb1WK360FRG6XO+SS87RhBoFufoHbM8JdfUMOUCLJj IpHw== X-Gm-Message-State: AKGB3mK8p8ViNHXypL06zzcXn4FsD1JK79JoNwg+bC1BGPR2PFKpD7qp P8GzN9/SiKuhTZSTjIyruiT5P+h6+e0= X-Google-Smtp-Source: ACJfBouf+1PcVPMAqOaOLZryHcOQPLPKVJkIiMddkN1YIfpCw09PQWdnS4facdx6/OMNGjvA2TqA7g== X-Received: by 10.223.139.73 with SMTP id v9mr33061913wra.77.1514493435068; Thu, 28 Dec 2017 12:37:15 -0800 (PST) Received: from localhost ([95.145.138.186]) by smtp.gmail.com with ESMTPSA id m134sm19698224wmg.6.2017.12.28.12.37.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 28 Dec 2017 12:37:14 -0800 (PST) From: Richard Sandiford To: Andreas Schwab Mail-Followup-To: Andreas Schwab ,gcc-patches@gcc.gnu.org, richard.sandiford@linaro.org Cc: gcc-patches@gcc.gnu.org Subject: RFA: Fix REG_ARGS_SIZE handling when pushing TLS addresses References: <871sltvm7r.fsf@linaro.org> <87h8upn5nm.fsf@linaro.org> <877etdhjl5.fsf@linaro.org> Date: Thu, 28 Dec 2017 20:37:00 -0000 In-Reply-To: (Andreas Schwab's message of "Sun, 24 Dec 2017 13:49:25 +0100") Message-ID: <87h8saio7a.fsf_-_@linaro.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2017-12/txt/msg01570.txt.bz2 Andreas Schwab writes: > On Dez 23 2017, Richard Sandiford wrote: >> gcc/ >> * expr.c (fixup_args_size_notes): Check that any existing >> REG_ARGS_SIZE notes are correct, and don't try to re-add them. >> (emit_single_push_insn_1): Move stack_pointer_delta adjustment to... >> (emit_single_push_insn): ...here. > > Successfully regtested on m68k-linux. Thanks. Now also tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu (not that that will give mucn coverage). Also tested with a before-and-after comparison of testsuite output for a range of targets. OK to install? Richard The new assert in add_args_size_note triggered for gcc.dg/tls/opt-3.c and other on m68k. This looks like a pre-existing bug: if we pushed a value that needs a call to something like __tls_get_addr, we ended up with two different REG_ARGS_SIZE notes on the same instruction. It seems to be OK for emit_single_push_insn to push something that needs a call to __tls_get_addr: /* We have to allow non-call_pop patterns for the case of emit_single_push_insn of a TLS address. */ if (GET_CODE (pat) != PARALLEL) return 0; so I think the bug is in the way this is handled rather than the fact that it occurs at all. If we're pushing a value X that needs a call C to calculate, we'll add REG_ARGS_SIZE notes to the pushes and pops for C as part of the call sequence. Then emit_single_push_insn calls fixup_args_size_notes on the whole push sequence (the calculation of X, including C, and the push of X itself). This is where the double notes came from. But emit_single_push_insn_1 adjusted stack_pointer_delta *before* the push, so the notes added for C were relative to the situation after the future push of X rather than before it. Presumably this didn't matter in practice because the note added second tended to trump the note added first. But code is allowed to walk REG_NOTES without having to disregard secondary notes. 2017-12-23 Richard Sandiford gcc/ * expr.c (fixup_args_size_notes): Check that any existing REG_ARGS_SIZE notes are correct, and don't try to re-add them. (emit_single_push_insn_1): Move stack_pointer_delta adjustment to... (emit_single_push_insn): ...here. Index: gcc/expr.c =================================================================== --- gcc/expr.c 2017-12-23 09:29:20.226338285 +0000 +++ gcc/expr.c 2017-12-23 09:29:45.783339673 +0000 @@ -4089,6 +4089,14 @@ fixup_args_size_notes (rtx_insn *prev, r if (!NONDEBUG_INSN_P (insn)) continue; + /* We might have existing REG_ARGS_SIZE notes, e.g. when pushing + a call argument containing a TLS address that itself requires + a call to __tls_get_addr. The handling of stack_pointer_delta + in emit_single_push_insn is supposed to ensure that any such + notes are already correct. */ + rtx note = find_reg_note (insn, REG_ARGS_SIZE, NULL_RTX); + gcc_assert (!note || known_eq (args_size, get_args_size (note))); + poly_int64 this_delta = find_args_size_adjust (insn); if (known_eq (this_delta, 0)) { @@ -4102,7 +4110,8 @@ fixup_args_size_notes (rtx_insn *prev, r if (known_eq (this_delta, HOST_WIDE_INT_MIN)) saw_unknown = true; - add_args_size_note (insn, args_size); + if (!note) + add_args_size_note (insn, args_size); if (STACK_GROWS_DOWNWARD) this_delta = -poly_uint64 (this_delta); @@ -4126,7 +4135,6 @@ emit_single_push_insn_1 (machine_mode mo rtx dest; enum insn_code icode; - stack_pointer_delta += PUSH_ROUNDING (GET_MODE_SIZE (mode)); /* If there is push pattern, use it. Otherwise try old way of throwing MEM representing push operation to move expander. */ icode = optab_handler (push_optab, mode); @@ -4213,6 +4221,14 @@ emit_single_push_insn (machine_mode mode emit_single_push_insn_1 (mode, x, type); + /* Adjust stack_pointer_delta to describe the situation after the push + we just performed. Note that we must do this after the push rather + than before the push in case calculating X needs pushes and pops of + its own (e.g. if calling __tls_get_addr). The REG_ARGS_SIZE notes + for such pushes and pops must not include the effect of the future + push of X. */ + stack_pointer_delta += PUSH_ROUNDING (GET_MODE_SIZE (mode)); + last = get_last_insn (); /* Notice the common case where we emitted exactly one insn. */