Re: m68k: Add __attribute__((interrupt)).

[Bernardo Innocenti <bernie at develer dot com>]

On Thursday 21 August 2003 07:33, Richard Henderson wrote:
> On Thu, Aug 21, 2003 at 04:06:52AM +0200, Bernardo Innocenti wrote:
> > +  if (interrupt_handler)
> > +    fprintf (stream, "\trte\n");
>
> Refresh my memory: Use of rte implies there is a proper exception frame on
> the stack, correct?

Yes..

> If so, that implies that the function cannot be called directly with the normal
> calling sequence.

Also correct.

> That implies that you are  missing code to either
> (1) create a fake exception frame for calling these functions or

I think this is intended for functions callable directly from interrupt
vectors without using assembly stubs.


> (2) deny that they can be called directly at all,

Issuing a warning might be useful, but how could I intercept function
calls in the back-end?


> (3) in either case, the attribute needs to be attached to the function type
> and not the function decl.

I think this is intended to work like this:


  void serial_interrupt_handler(void) __attribute__((interrupt))
  {
      ...
  }


> > -  return (regs_ever_live[regno]
> > -	  && !call_used_regs[regno]
> > +  return (
> > +	  ((regs_ever_live[regno] && !call_used_regs[regno])
> > +	    || (interrupt_handler
> > +	      && (regs_ever_live[regno]
> > +	        || (call_used_regs[regno] && !current_function_is_leaf)
> > +	      )
> > +	    )
> > +	  )
> >  	  && !fixed_regs[regno]
> >  	  && !(regno == FRAME_POINTER_REGNUM && frame_pointer_needed));
>
> This change can't possibly be correct.  What has current_function_is_leaf
> got to do with anything? 

I think the spirit is to save callee-save registers (a0/a1/d0/d1) if
they are being used _OR_ if the interrupt function calls other functions
that might clobber them without us knowing.


> Seems to me the logic should be 
>
>   /* Fixed regs we never touch.  */
>   if (fixed_regs[regno])
>     return false;
>   /* The frame pointer (if it is such) is handled specially.  */
>   if (regno == FRAME_POINTER_REGNUM && frame_pointer_needed)
>     return false;
>   /* Never need to save registers that aren't touched.  */
>   if (!regs_ever_live[regno])
>     return false;
>   /* Interrupt functions save everything else.  */
>   if (interrupt_handler)
>     return true;
>   /* Otherwise save everthing that isn't call-clobbered.  */
>   return !call_used_regs[regno];

If my interpretation above was correct, this last piece needs to
be done like this:

   /* Fixed regs we never touch.  */
   if (fixed_regs[regno])
     return false;

   /* The frame pointer (if it is such) is handled specially.  */
   if (regno == FRAME_POINTER_REGNUM && frame_pointer_needed)
     return false;

   /* Interrupt handlers must also save call_used_regs
      if they are live or when calling nested functions.  */
   if (interrupt_handler)
   {
      if (regs_ever_live[regno])
        return true;

      if (!current_function_is_leaf && call_used_regs[regno])
        return true;
   }

   /* Never need to save registers that aren't touched.  */
   if (!regs_ever_live[regno])
     return false;

   /* Otherwise save everthing that isn't call-clobbered.  */
   return !call_used_regs[regno];


> Which, incidentaly, seems actually readable rather than huge
> nested conditionals.  Much easier to verify.

I also hate complex nested expressions, but they used to be
quite popular in GCC, so I didn't mind converting it to
something more readable :-)

I will re-submit the patch after testing these changes. Thank you!

-- 
  // Bernardo Innocenti - Develer S.r.l., R&D dept.
\X/  http://www.develer.com/

Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html