Patchwork PATCH: Properly check mode for x86 call/jmp address

login
register
mail settings
Submitter Uros Bizjak
Date March 7, 2012, 9:28 a.m.
Message ID <CAFULd4Z9wo0dyvUr5bey-yHM-597TQm7_1cvtE_j_yviajGG_A@mail.gmail.com>
Download mbox | patch
Permalink /patch/145173/
State New
Headers show

Comments

Uros Bizjak - March 7, 2012, 9:28 a.m.
On Tue, Mar 6, 2012 at 9:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>  (define_insn "*call"
>>> -  [(call (mem:QI (match_operand:P 0 "call_insn_operand" "<c>zw"))
>>> +  [(call (mem:QI (match_operand:C 0 "call_insn_operand" "<c>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.
Uros Bizjak - March 7, 2012, 10:07 a.m.
On Wed, Mar 7, 2012 at 10:28 AM, Uros Bizjak <ubizjak@gmail.com> wrote:

> +  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.

Ah, I vaguely remember that indirect call/jmp is invalid on X32 for
some other reason. So, please leave the condition above as is and also
revert similar change in attached patch back to (not (match_test
"TARGET_X32")).

Uros.
Uros Bizjak - March 7, 2012, 10:22 a.m.
On Wed, Mar 7, 2012 at 11:07 AM, Uros Bizjak <ubizjak@gmail.com> wrote:

> Ah, I vaguely remember that indirect call/jmp is invalid on X32 for

This should read "... indirect call/jmp FROM MEMORY is invalid on X32
...". It looks I've had too much morning coffee already ;)

Uros.
H.J. Lu - March 7, 2012, 4:03 p.m.
On Wed, Mar 7, 2012 at 1:28 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Mar 6, 2012 at 9:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>  (define_insn "*call"
>>>> -  [(call (mem:QI (match_operand:P 0 "call_insn_operand" "<c>zw"))
>>>> +  [(call (mem:QI (match_operand:C 0 "call_insn_operand" "<c>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 <http://gcc.gnu.org/bugs.html> for instructions.
make: *** [x.s] Error 1

I will investigate it.

Patch

Index: predicates.md
===================================================================
--- predicates.md	(revision 184992)
+++ 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.
@@ -557,22 +565,27 @@ 
        (match_operand 0 "immediate_operand")))
 
 ;; Test for a valid operand for indirect branch.
+;; Allow register operands in word mode only.
 (define_predicate "indirect_branch_operand"
-  (if_then_else (match_test "TARGET_X32")
-    (match_operand 0 "register_operand")
-    (match_operand 0 "nonimmediate_operand")))
+  (ior (match_test "register_operand
+		     (op, mode == VOIDmode ? mode : word_mode)")
+       (and (match_test "Pmode == word_mode")
+	    (match_operand 0 "memory_operand"))))
 
 ;; Test for a valid operand for a call instruction.
+;; Allow register operands in word mode only.
 (define_predicate "call_insn_operand"
   (ior (match_operand 0 "constant_call_address_operand")
-       (match_operand 0 "call_register_no_elim_operand")
-       (and (not (match_test "TARGET_X32"))
+       (match_test "call_register_no_elim_operand
+		     (op, mode == VOIDmode ? mode : word_mode)")
+       (and (match_test "Pmode == word_mode")
 	    (match_operand 0 "memory_operand"))))
 
 ;; Similarly, but for tail calls, in which we cannot allow memory references.
 (define_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"