From patchwork Wed Mar 7 21:58:03 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Uros Bizjak X-Patchwork-Id: 145345 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 46FD0B6F62 for ; Thu, 8 Mar 2012 08:58:23 +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=1331762303; h=Comment: DomainKey-Signature:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Cc:Content-Type:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=BwlTGL7cl4AfQtiXpdpNG4I8mWc=; b=jhA8VC/4nI2bRpk6eOb6RoSSJvMZ3tP8AK4InK5kpOL+9rZDXgKA+cgvMfBWbK iX9iaEnVravB17rUMVDYX7umiOFFirn6dW8cSrBkDrjeU0hO0AxEQLfg+Pn5l/yh q2Ahzq9NDzGBrb1zCJK/xs+FnfXK45YxYwE6ndj+COA50= 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:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=e1sWPOBfZxPrYrxt0sifm0nShEC7m8yhNuF/o4GoPpazqCVmUAuSR2Gb68CrIa rHiKh5ruiYIXNU5PTf+/UTznurIm5hNg1IwxH0WokILqkWtvzbAwL2VKcC/QMxmw xzsZAGPSfLFCxIfsgLjcn0SJuwwTRYINpHS48+zZa8Anw=; Received: (qmail 20458 invoked by alias); 7 Mar 2012 21:58:19 -0000 Received: (qmail 20436 invoked by uid 22791); 7 Mar 2012 21:58:17 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-yw0-f47.google.com (HELO mail-yw0-f47.google.com) (209.85.213.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Mar 2012 21:58:03 +0000 Received: by yhjj56 with SMTP id j56so3484606yhj.20 for ; Wed, 07 Mar 2012 13:58:03 -0800 (PST) MIME-Version: 1.0 Received: by 10.236.46.232 with SMTP id r68mr7351833yhb.80.1331157483254; Wed, 07 Mar 2012 13:58:03 -0800 (PST) Received: by 10.146.241.19 with HTTP; Wed, 7 Mar 2012 13:58:03 -0800 (PST) In-Reply-To: References: <4F5669D2.8080805@redhat.com> Date: Wed, 7 Mar 2012 22:58:03 +0100 Message-ID: Subject: Re: PATCH: Properly check mode for x86 call/jmp address From: Uros Bizjak To: "H.J. Lu" Cc: Richard Henderson , gcc-patches@gcc.gnu.org, Jakub Jelinek 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 On Wed, Mar 7, 2012 at 5:03 PM, H.J. Lu wrote: >>>>>  (define_insn "*call" >>>>> -  [(call (mem:QI (match_operand:P 0 "call_insn_operand" "zw")) >>>>> +  [(call (mem:QI (match_operand:C 0 "call_insn_operand" "zw")) >>>>>        (match_operand 1 "" ""))] >>>>> -  "!SIBLING_CALL_P (insn)" >>>>> +  "!SIBLING_CALL_P (insn) >>>>> +   && (GET_CODE (operands[0]) == SYMBOL_REF >>>>> +       || GET_MODE (operands[0]) == word_mode)" >>>> >>>> There are enough copies of this extra constraint that I wonder >>>> if it simply ought to be folded into call_insn_operand. >>>> >>>> Which would need to be changed to define_special_predicate, >>>> since you'd be doing your own mode checking. >>>> >>>> Probably similar changes to sibcall_insn_operand. >>> >>> Here is the updated patch.  I changed constant_call_address_operand >>> and call_register_no_elim_operand to use define_special_predicate. >>> OK for trunk? >> >> Please do not complicate matters that much. Just stick word_mode >> overrides for register operands in predicates.md, like in attached >> patch. These changed predicates now allow registers only in word_mode >> (and VOIDmode). >> >> You can now remove all new mode iterators and leave call patterns untouched. >> >> @@ -22940,14 +22940,18 @@ ix86_expand_call (rtx retval, rtx fnaddr, >> rtx callarg1, >>       && GET_CODE (XEXP (fnaddr, 0)) == SYMBOL_REF >>       && !local_symbolic_operand (XEXP (fnaddr, 0), VOIDmode)) >>     fnaddr = gen_rtx_MEM (QImode, construct_plt_address (XEXP (fnaddr, 0))); >> -  else if (sibcall >> -          ? !sibcall_insn_operand (XEXP (fnaddr, 0), Pmode) >> -          : !call_insn_operand (XEXP (fnaddr, 0), Pmode)) >> +  else if (!(constant_call_address_operand (XEXP (fnaddr, 0), Pmode) >> +            || call_register_no_elim_operand (XEXP (fnaddr, 0), >> +                                              word_mode) >> +            || (!sibcall >> +                && !TARGET_X32 >> +                && memory_operand (XEXP (fnaddr, 0), word_mode)))) >>     { >>       fnaddr = XEXP (fnaddr, 0); >> -      if (GET_MODE (fnaddr) != Pmode) >> -       fnaddr = convert_to_mode (Pmode, fnaddr, 1); >> -      fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (Pmode, fnaddr)); >> +      if (GET_MODE (fnaddr) != word_mode) >> +       fnaddr = convert_to_mode (word_mode, fnaddr, 1); >> +      fnaddr = gen_rtx_MEM (QImode, >> +                           copy_to_mode_reg (word_mode, fnaddr)); >>     } >> >>   vec_len = 0; >> >> Please update the above part. It looks you don't even have to change >> condition with new predicates. Basically, you should only convert the >> address to word_mode instead of Pmode. >> >> +  if (TARGET_X32) >> +    operands[0] = convert_memory_address (word_mode, operands[0]); >> >> This addition to indirect_jump and tablejump should be the only >> change, needed in i386.md now. Please write the condition >> >> if (Pmode != word_mode) >> >> for consistency. >> >> BTW: The attached patch was bootstrapped and regression tested on >> x86_64-pc-linux-gnu {,-m32}. >> >> Uros. > > It doesn't work: > > x.i:7:1: error: unrecognizable insn: > (call_insn/j 8 7 9 3 (call (mem:QI (reg:DI 62) [0 *foo.0_1 S1 A8]) >        (const_int 0 [0])) x.i:6 -1 >     (nil) >    (nil)) > x.i:7:1: internal compiler error: in extract_insn, at recog.c:2123 > Please submit a full bug report, > with preprocessed source if appropriate. > See for instructions. > make: *** [x.s] Error 1 > > I will investigate it. For reference, attached is the complete patch that uses define_special_predicate. This patch works OK with the current mainline, with additional patch to i386.h, where Uros. Index: i386.c =================================================================== --- i386.c (revision 185058) +++ i386.c (working copy) @@ -22952,9 +22952,9 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx call : !call_insn_operand (XEXP (fnaddr, 0), Pmode)) { fnaddr = XEXP (fnaddr, 0); - if (GET_MODE (fnaddr) != Pmode) - fnaddr = convert_to_mode (Pmode, fnaddr, 1); - fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (Pmode, fnaddr)); + if (GET_MODE (fnaddr) != word_mode) + fnaddr = convert_to_mode (word_mode, fnaddr, 1); + fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (word_mode, fnaddr)); } vec_len = 0; Index: i386.md =================================================================== --- i386.md (revision 185073) +++ i386.md (working copy) @@ -11078,7 +11078,12 @@ (set_attr "modrm" "0")]) (define_expand "indirect_jump" - [(set (pc) (match_operand 0 "indirect_branch_operand" ""))]) + [(set (pc) (match_operand 0 "indirect_branch_operand" ""))] + "" +{ + if (TARGET_X32) + operands[0] = convert_memory_address (word_mode, operands[0]); +}) (define_insn "*indirect_jump" [(set (pc) (match_operand:P 0 "indirect_branch_operand" "rw"))] @@ -11123,8 +11128,9 @@ operands[0] = expand_simple_binop (Pmode, code, op0, op1, NULL_RTX, 0, OPTAB_DIRECT); } - else if (TARGET_X32) - operands[0] = convert_memory_address (Pmode, operands[0]); + + if (TARGET_X32) + operands[0] = convert_memory_address (word_mode, operands[0]); }) (define_insn "*tablejump_1" Index: predicates.md =================================================================== --- predicates.md (revision 185073) +++ predicates.md (working copy) @@ -1,5 +1,5 @@ ;; Predicate definitions for IA-32 and x86-64. -;; Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 +;; Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 ;; Free Software Foundation, Inc. ;; ;; This file is part of GCC. @@ -565,22 +565,27 @@ (match_operand 0 "immediate_operand"))) ;; Test for a valid operand for indirect branch. -(define_predicate "indirect_branch_operand" - (if_then_else (match_test "TARGET_X32") - (match_operand 0 "register_operand") - (match_operand 0 "nonimmediate_operand"))) +;; Allow register operands in word mode only. +(define_special_predicate "indirect_branch_operand" + (ior (match_test "register_operand + (op, mode == VOIDmode ? mode : word_mode)") + (and (not (match_test "TARGET_X32")) + (match_operand 0 "memory_operand")))) ;; Test for a valid operand for a call instruction. -(define_predicate "call_insn_operand" +;; Allow register operands in word mode only. +(define_special_predicate "call_insn_operand" (ior (match_operand 0 "constant_call_address_operand") - (match_operand 0 "call_register_no_elim_operand") + (match_test "call_register_no_elim_operand + (op, mode == VOIDmode ? mode : word_mode)") (and (not (match_test "TARGET_X32")) (match_operand 0 "memory_operand")))) ;; Similarly, but for tail calls, in which we cannot allow memory references. -(define_predicate "sibcall_insn_operand" +(define_special_predicate "sibcall_insn_operand" (ior (match_operand 0 "constant_call_address_operand") - (match_operand 0 "register_no_elim_operand"))) + (match_test "register_no_elim_operand + (op, mode == VOIDmode ? mode : word_mode)"))) ;; Match exactly zero. (define_predicate "const0_operand" Index: i386.h =================================================================== --- i386.h (revision 185079) +++ i386.h (working copy) @@ -1744,7 +1744,7 @@ /* Specify the machine mode that pointers have. After generation of rtl, the compiler makes no further distinction between pointers and any other objects of this machine mode. */ -#define Pmode (TARGET_64BIT ? DImode : SImode) +#define Pmode (TARGET_LP64 ? DImode : SImode) /* A C expression whose value is zero if pointers that need to be extended from being `POINTER_SIZE' bits wide to `Pmode' are sign-extended and