Re: [PATCH] x86 interrupt attribute

[Yulia Koval <vaalfreja at gmail dot com>]

The debug_info section for the interrupt function looks ok.

I tried to call it from assembler code to check it in gdb.

        pushl   $0x333 ;eflags
        pushl   $0x111 ;cs
        pushl $0x222   ;eip
        jmp  foo           ;interrupt function

#define uword_t unsigned int
struct interrupt_frame
{
     uword_t ip;
     uword_t cs;
     uword_t flags;
};

I get this inside the interrupt function:

Breakpoint 1, foo (frame=0xffffd560) at interrupt-1.c:7
7               a = (struct interrupt_frame*) frame;
(gdb) p/x ((struct interrupt_frame*)frame)->ip
$1 = 0x222
(gdb) p/x ((struct interrupt_frame*)frame)->cs
$3 = 0x111
(gdb) p/x ((struct interrupt_frame*)frame)->flags
$4 = 0x333

Frame pointer info looks ok.


On Tue, Oct 13, 2015 at 3:18 PM, Yulia Koval <vaalfreja@gmail.com> wrote:
>
> Here is the current version of the patch with all the fixes.
> Regtested\bootstraped it on 64 bit.
>
> We need a pointer since interrupt handler will update data pointing
> to by frame.  Since error_code isn't at the normal location where the
> parameter is passed on stack and frame isn't in a hard register, we
> changed ix86_function_arg:
>
> +  if (!cum->caller && cfun->machine->func_type != TYPE_NORMAL)
> +    {
> +      /* The first argument of interrupt handler is a pointer and
> +        points to the return address slot on stack.  The optional
> +        second argument is an integer for error code on stack.  */
> +      gcc_assert (type != NULL_TREE);
> +      if (POINTER_TYPE_P (type))
> +       {
> +         if (cfun->machine->func_type == TYPE_EXCEPTION)
> +           /* (AP) in the current frame in exception handler.  */
> +           arg = arg_pointer_rtx;
> +         else
> +           /* -WORD(AP) in the current frame in interrupt handler.  */
> +           arg = force_reg (Pmode,
> +                            plus_constant (Pmode, arg_pointer_rtx,
> +                                           -UNITS_PER_WORD));
> +         if (mode != Pmode)
> +           arg = convert_to_mode (mode, arg, 1);
> +       }
> +      else
> +       {
> +         gcc_assert (TREE_CODE (type) == INTEGER_TYPE
> +                     && cfun->machine->func_type == TYPE_EXCEPTION
> +                     && mode == word_mode);
> +         /* The error code is at -WORD(AP) in the current frame in
> +            exception handler.  */
> +         arg = gen_rtx_MEM (word_mode,
> +                            plus_constant (Pmode, arg_pointer_rtx,
> +                                           -UNITS_PER_WORD));
> +       }
> +
> +      return arg;
> +    }
> +
>
> to return a pseudo register.  It violates
>
>    Return where to put the arguments to a function.
>    Return zero to push the argument on the stack, or a hard register in
>    which to store the argument.
>
> Register allocator has no problem with parameters in pseudo registers.
> But GCC crashes when it tries to access DECL_INCOMING_RTL as a hard
> register when generating debug information.  We worked around it by
> doing
>
> +
> +  if (cfun->machine->func_type != TYPE_NORMAL)
> +    {
> +      /* Since the pointer argument of interrupt handler isn't a real
> +         argument, adjust DECL_INCOMING_RTL for debug output.  */
> +      tree arg = DECL_ARGUMENTS (current_function_decl);
> +      gcc_assert (arg != NULL_TREE
> +                 && POINTER_TYPE_P (TREE_TYPE (arg)));
> +      if (cfun->machine->func_type == TYPE_EXCEPTION)
> +       /* (AP) in the current frame in exception handler.  */
> +       DECL_INCOMING_RTL (arg) = arg_pointer_rtx;
> +      else
> +       /* -WORD(AP) in the current frame in interrupt handler.  */
> +       DECL_INCOMING_RTL (arg) = plus_constant (Pmode,
> +                                                arg_pointer_rtx,
> +                                                -UNITS_PER_WORD);
> +    }
>
>
> On Mon, Oct 5, 2015 at 12:29 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Mon, Oct 5, 2015 at 1:17 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> >>>>>>> Looking a bit deeper into the code, it looks that we want to realign
> >>>>>>> the stack in the interrupt handler. Let's assume that interrupt
> >>>>>>> handler is calling some other function that saves SSE vector regs to
> >>>>>>> the stack. According to the x86 ABI, incoming stack of the called
> >>>>>>> function is assumed to be aligned to 16 bytes. But, interrupt handler
> >>>>>>> violates this assumption, since the stack could be aligned to only 4
> >>>>>>> bytes for 32bit and 8 bytes for 64bit targets. Entering the called
> >>>>>>> function with stack, aligned to less than 16 bytes will certainly
> >>>>>>> violate ABI.
> >>>>>>>
> >>>>>>> So, it looks to me that we need to realign the stack in the interrupt
> >>>>>>> handler unconditionally to 16bytes. In this case, we also won't need
> >>>>>>> the following changes:
> >>>>>>>
> >>>>>>
> >>>>>> Current stack alignment implementation requires at least
> >>>>>> one, maybe two, scratch registers:
> >>>>>>
> >>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67841
> >>>>>>
> >>>>>> Extend it to the interrupt handler, which doesn't have any scratch
> >>>>>> registers may require significant changes in backend as well as
> >>>>>> register allocator.
> >>>>>
> >>>>> But without realignment, the handler is unusable for anything but
> >>>>> simple functions. The handler will crash when called function will try
> >>>>> to save vector reg to stack.
> >>>>>
> >>>>
> >>>> We can use unaligned load and store to avoid crash.
> >>>
> >>> Oh, sorry, I meant "called function will crash", like:
> >>>
> >>> -> interrupt when %rsp = 0x...8 ->
> >>> -> interrupt handler ->
> >>> -> calls some function that tries to save xmm reg to stack
> >>> -> crash in the called function
> >>>
> >>
> >> It should be fixed by this patch.   But we need to fix stack
> >> alignment in interrupt handler to avoid scratch register.
> >>
> >>
> >> --
> >> H.J.
> >> ---
> >> commit 15f48be1dc7ff48207927d0b835e593d058f695b
> >> Author: H.J. Lu <hjl.tools@gmail.com>
> >> Date:   Sun Oct 4 16:14:03 2015 -0700
> >>
> >>     Correctly set incoming stack boundary for interrupt handler
> >>
> >> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> >> index 7ebdcd9..0f0cc3c 100644
> >> --- a/gcc/config/i386/i386.c
> >> +++ b/gcc/config/i386/i386.c
> >> @@ -12037,8 +12037,11 @@ ix86_minimum_incoming_stack_boundary (bool sibcall)
> >>  {
> >>    unsigned int incoming_stack_boundary;
> >>
> >> +  /* Stack of interrupt handler is always aligned to word_mode.  */
> >> +  if (cfun->machine->func_type != TYPE_NORMAL)
> >> +    incoming_stack_boundary = TARGET_64BIT ? 64 : 32;
> >
> > Just a heads up that in order to support stack realignmnent on x86_64,
> > MIN_STACK_BOUNDARY will soon be changed to BITS_PER_WORD, so you can
> > use it in the line above. Please see comment #5 and #6 of PR 66697
> > [1].
> >
> >>    /* Prefer the one specified at command line. */
> >> -  if (ix86_user_incoming_stack_boundary)
> >> +  else if (ix86_user_incoming_stack_boundary)
> >>      incoming_stack_boundary = ix86_user_incoming_stack_boundary;
> >>    /* In 32bit, use MIN_STACK_BOUNDARY for incoming stack boundary
> >>       if -mstackrealign is used, it isn't used for sibcall check and
> >
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66697
> >
> > Uros.

Attachment: patch_repost
Description: Binary data