From patchwork Tue Oct 18 16:41:45 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Julian Brown X-Patchwork-Id: 120462 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 1114AB71BE for ; Wed, 19 Oct 2011 03:42:23 +1100 (EST) Received: (qmail 1913 invoked by alias); 18 Oct 2011 16:42:18 -0000 Received: (qmail 1683 invoked by uid 22791); 18 Oct 2011 16:42:15 -0000 X-SWARE-Spam-Status: No, hits=-0.9 required=5.0 tests=AWL, BAYES_00, FROM_12LTRDOM, TO_NO_BRKTS_PCNT X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 18 Oct 2011 16:41:56 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RGCjj-0000AZ-6l from Julian_Brown@mentor.com for gcc-patches@gcc.gnu.org; Tue, 18 Oct 2011 09:41:55 -0700 Received: from rex.config ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Tue, 18 Oct 2011 17:41:53 +0100 Date: Tue, 18 Oct 2011 17:41:45 +0100 From: Julian Brown To: gcc-patches@gcc.gnu.org Subject: [PATCH] Fix computed gotos on m68k Message-ID: <20111018174145.4d7cf3bc@rex.config> Mime-Version: 1.0 X-IsSubscribed: yes 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 Hi, This patch fixes computed gotos on m68k, and probably other targets too (the fix is in the middle end). Several tests fail at present with "-O3 -fomit-frame-pointer". One example of erroneous behaviour is as follows: the function 'x' in comp-goto-1.c compiles to: x: lea (-104,%sp),%sp lea (104,%sp),%a0 move.l %a0,92(%sp) fmovem #63,44(%sp) movem.l #31996,(%sp) move.l %sp,96(%sp) move.l 108(%sp),-(%sp) lea (96,%sp),%a0 jsr y.1217 .L3: .L12: move.l 112(%sp),%d0 <--- wrong offset, should be 108. movem.l (%sp),#31996 fmovem 44(%sp),#63 lea (104,%sp),%sp rts Where the source code looks like: int x(a) { __label__ xlab; void y(a) { void *x = &&llab; if (a==-1) goto *x; if (a==0) goto xlab; llab: y (a-1); } y (a); xlab:; return a; } The offset is intended to load the argument value 'a' for the "return a;" statement, but actually loads one word beyond that. This has been broken because since mainline revision r160124, which (correctly) causes 'y' to be flagged as noreturn -- y never returns via the normal function return route. If ipa-pure-const.c is hacked to check that a function does not perform non-local gotos before marking it noreturn, the original (correct) offset is restored -- but that is simply papering over the problem. Prior to r160124, or with the aforementioned patch to inhibit the "noreturn" attribute for the function y, the call to y from x (just before "xlab:") has a goto to the following label emitted at expand time. Before/after that patch is applied, the diff of 148r.expand around the relevant part looks like: # BLOCK 2 freq:10000 # PRED: ENTRY [100.0%] (fallthru,exec) y (a_1(D)); [static-chain: &FRAME.0] - # SUCC: 3 [100.0%] (ab,exec) + goto (xlab); + # SUCC: 3 [61.0%] (ab,exec) 4 [39.0%] (fallthru,exec) - # BLOCK 3 freq:10000 - # PRED: 2 [100.0%] (ab,exec) + # BLOCK 3 freq:6100 + # PRED: 2 [61.0%] (ab,exec) : [non-local] # SUCC: 4 [100.0%] (fallthru,exec) # BLOCK 4 freq:10000 - # PRED: 3 [100.0%] (fallthru,exec) + # PRED: 2 [39.0%] (fallthru,exec) 3 [100.0%] (fallthru,exec) xlab: Now, this is important because when the "goto " is emitted, a stack adjustment is also emitted to pop y's arguments (via dojump.c:do_pending_stack_adjust). Without that (i.e. the present state with no patches), when later passes try to figure out elimination offsets (from arg pointer to frame pointer, frame pointer to stack pointer and so on) at the labels L0/xlab, they get the wrong answer. (This happens in reload, called from ira.) So, on to the attached fix. As mentioned previously, GCC tracks elimination offsets throughout each function, which can vary as e.g. other functions are called. There is a concept of an "initial" offset: I believe that when nonlocal gotos are executed, and hence at all the labels a nonlocal goto may reach, the elimination offsets must have their "initial" values. We can find out if a label is the target of a nonlocal goto (and hence force the use of initial offsets) in a somewhat roundabout way, by looking up its containing basic block, seeing if that BB is a nonlocal goto target, then seeing if the label is the first instruction in the block. This seems slightly clumsy, but I didn't find another way of doing it. Interestingly perhaps, the comment near the fix in reload1.c:set_label_offsets hints at a possible issue which may be related: case CODE_LABEL: /* If we know nothing about this label, set the desired offsets. Note that this sets the offset at a label to be the offset before a label if we don't know anything about the label. This is not correct for the label after a BARRIER, but is the best guess we can make. If we guessed wrong, we will suppress an elimination that might have been possible had we been able to guess correctly. */ But obviously, in our case, rather than just failing to make an elimination which may have been possible, we get wrong code instead (there is a barrier before our non-local-goto-receiving label). Anyway. The attached patch appears to work fine, and we get the following changes in test results (cross-building and testing to ColdFire Linux): FAIL -> PASS: gcc.c-torture/execute/920501-7.c execution, -O3 -fomit-frame-pointer FAIL -> PASS: gcc.c-torture/execute/comp-goto-2.c execution, -O3 -fomit-frame-pointer FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c -O3 -fomit-frame-pointer execution test FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c -O3 -fomit-frame-pointer execution test [#2] FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c -O3 -fomit-frame-pointer execution test FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c -O3 -fomit-frame-pointer execution test [#2] Any comments, or OK to apply? Thanks, Julian ChangeLog gcc/ * reload1.c (set_label_offsets): Force initial_p for labels receiving non-local gotos. Index: gcc/reload1.c =================================================================== --- gcc/reload1.c (revision 179967) +++ gcc/reload1.c (working copy) @@ -2371,6 +2371,19 @@ set_label_offsets (rtx x, rtx insn, int if (! offsets_known_at[CODE_LABEL_NUMBER (x) - first_label_num]) { + if (x == insn) + { + basic_block bb; + + bb = BLOCK_FOR_INSN (insn); + + /* If the label is the target of a non-local GOTO, we must use + the initial elimination offsets. */ + if (bb && BB_HEAD (bb) == insn + && (bb->flags & BB_NON_LOCAL_GOTO_TARGET)) + initial_p = true; + } + for (i = 0; i < NUM_ELIMINABLE_REGS; i++) offsets_at[CODE_LABEL_NUMBER (x) - first_label_num][i] = (initial_p ? reg_eliminate[i].initial_offset