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

[Sandra Loosemore <sandra at codesourcery dot com>]

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