RE: [PATCH] x86 interrupt attribute patch [2/2]

["Koval, Julia" <julia dot koval at intel dot com>]

> I'm also confused; I can see how an interrupt handler might have different register usage conventions, but do regular functions called from inside an interrupt handler function really use a non-standard call/return sequence?
Usually, they don't. But because of this, the interrupt handler should save all the context, because any register can be used in the called function and we have no information about it. This is bad for performance. That's why no_caller_saved_registers attribute was proposed - the function with this attribute saves/restores all modified registers itself, and an interrupt function can call it without saving full context. Here is the discussion: https://gcc.gnu.org/ml/gcc-patches/2015-09/msg02277.html

Here is the patch with the fixes. Ok for trunk?

Thanks,
Julia

-----Original Message-----
From: Sandra Loosemore [mailto:sandra@codesourcery.com] 
Sent: Monday, May 30, 2016 10:37 PM
To: Koval, Julia <julia.koval@intel.com>; gcc-patches@gcc.gnu.org
Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; vaalfreja@gmail.com; ubizjak@gmail.com; law@redhat.com; Zamyatin, Igor <igor.zamyatin@intel.com>
Subject: Re: [PATCH] x86 interrupt attribute patch [2/2]

On 05/30/2016 07:31 AM, Koval, Julia wrote:
> Hi,
> Here is the fixed version of the patch. Ok for trunk?
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 
> 2d4f028..f4bd7dd 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -5266,6 +5266,96 @@ On x86-32 targets, the @code{stdcall} attribute 
> causes th e compiler to  assume that the called function pops off the 
> stack space used to  pass arguments, unless it takes a variable number 
> of arguments.
>
> +@item no_caller_saved_registers
> +@cindex @code{no_caller_saved_registers} function attribute, x86 Use 
> +this attribute to indicate that the specified function has no 
> +caller-saved registers, for example for a function, called from an 
> +interrupt handler.  That is, all registers are callee-saved.

I think the "for example" information should come after the second sentence to improve the flow here.  I'm also confused; I can see how an interrupt handler might have different register usage conventions, but do regular functions called from inside an interrupt handler function really use a non-standard call/return sequence?

> +The compiler generates proper function entry and exit sequences to 
> +save and restore any modified registers, except for the EFLAGS 
> +register.  Since GCC doesn't preserve MPX, SSE, MMX nor x87 states, 
> +the GCC option, @option{-mgeneral-regs-only}, should be used to

Please delete both commas in the line above.

> +compile functions with @code{no_caller_saved_registers} attribute.
> +
> +@emph{Note for compiler implementers:} If the compiler generates MPX, 
> +SSE, MMX or x87 instructions in a function with 
> +@code{no_caller_saved_registers} attribute or functions called from a 
> +function with @code{no_caller_saved_registers} attribute may contain 
> +MPX, SSE, MMX or x87 instructions, the compiler must save and restore 
> +the corresponding state.

A "Note for compiler implementers" has no place in user documentation. 
You should just document what GCC does, if it is relevant to how a user would use this feature.

It also seems like the admonition in this note that the compiler must save/restore the state contradicts the previous paragraph, where you say GCC doesn't preserve the state.

> +
> +@item interrupt
> +@cindex @code{interrupt} function attribute, x86 Use this attribute 
> +to indicate that the specified function is an interrupt handler or an 
> +exception handler (depending on parameters, passed

Delete the comma.

> +to the function, explained further).  The compiler generates function 
> +entry and exit sequences suitable for use in an interrupt handler 
> +when this attribute is present.  The @code{IRET} instruction, instead 
> +of the @code{RET} instruction, is used to return from interrupt 
> +handlers.  All registers, except for the EFLAGS register which is 
> +restored by the @code{IRET} instruction, are preserved by the 
> +compiler.  Since GCC doesn't preserve MPX, SSE, MMX nor x87 states, 
> +the GCC option, @option{-mgeneral-regs-only}, should be used to 
> +compile interrupt and

Delete the two previous commas.

> +exception handlers.
> +
> +@emph{Note for compiler implementers:} If the compiler generates MPX, 
> +SSE, MMX or x87 instructions in an interrupt or exception handler, or 
> +functions called from an interrupt or exception handler may contain 
> +MPX, SSE, MMX or x87 instructions, the compiler must save and restore 
> +the corresponding state.

Again, this is user documentation.  Just explain what GCC does if it is relevant to how a user would use the feature you are documenting.

> +
> +Since the direction flag in the FLAGS register in interrupt handlers 
> +is undetermined, cld instruction must be emitted in function prologue 
> +if rep string instructions are used in interrupt handler or interrupt 
> +handler isn't a leaf function.

This paragraph seems totally implementor-speaky and irrelevant to how a user would use the feature.

> +
> +Any interruptible-without-stack-switch code must be compiled with 
> +@option{-mno-red-zone} since interrupt handlers can and will, because 
> +of the hardware design, touch the red zone.
> +
> +An interrupt handler must be declared with a mandatory pointer
> +argument:
> +
> +@smallexample
> +struct interrupt_frame;
> +
> +__attribute__ ((interrupt))
> +void
> +f (struct interrupt_frame *frame)
> +@{
> +@}
> +@end smallexample
> +
> +and you must define the structure the pointer pointing to, depending 
> +on the proper x86 interrupt frame, described in the processor's manual.

How about

@noindent
and you must define @code{struct interrupt_frame} as described in the processor's manual.


> +
> +The exception handler is very similar to the interrupt handler with
> +a different mandatory function signature:
> +
> +@smallexample
> +#ifdef __x86_64__
> +typedef unsigned long long int uword_t;
> +#else
> +typedef unsigned int uword_t;
> +#endif
> +
> +struct interrupt_frame;
> +
> +__attribute__ ((interrupt))
> +void
> +f (struct interrupt_frame *frame, uword_t error_code)
> +@{
> +  ...
> +@}
> +@end smallexample
> +
> +and compiler pops the error code off the stack before the @code{IRET}
> +instruction.
> +
> +The exception handler should only be used for exceptions which push an
> +error code and all other exceptions must use the interrupt handler.
> +The system will crash if the wrong handler is used.

I'm still not happy with the organization of material in this part.  How 
about something like this instead:

Exception handlers differ from interrupt handlers because the system 
pushes an error code on the stack.  An exception handler declaration is 
similar to that for an interrupt handler, but with a different mandatory 
function signature.  The compiler arranges to pop the error code off the 
stack before the @code{IRET} instruction.

[example here]

Exception handlers should only be used for exceptions that push an error 
code; you should use an interrupt handler in other cases.  The system 
will crash if the wrong kind of handler is used.

> +
>  @item target (@var{options})
>  @cindex @code{target} function attribute
>  As discussed in @ref{Common Function Attributes}, this attribute

-Sandra

Attachment: patch_01.06.patch
Description: patch_01.06.patch