Page MenuHomePhabricator

[X86] Support 'interrupt' attribute for x86
ClosedPublic

Authored by ABataev on Dec 21 2015, 10:20 PM.

Details

Summary

This attribute may be attached to a function definition and instructs the backend to generate appropriate function entry/exit code so that
it can be used directly as an interrupt handler.
The IRET instruction, instead of the RET instruction, is used to return from interrupt or exception handlers. All registers, except for the EFLAGS register which is restored by the IRET instruction, are preserved by the compiler.
Any interruptible-without-stack-switch code must be compiled with -mno-red-zone since interrupt handlers can and will, because of the hardware design, touch
the red zone.

  1. interrupt handler must be declared with a mandatory pointer argument:
struct interrupt_frame;

__attribute__ ((interrupt))
void f (struct interrupt_frame *frame) {
  ...
}

and user must properly define the structure the pointer pointing to.

  1. exception handler:

    The exception handler is very similar to the interrupt handler with a different mandatory function signature:
#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) {
  ...
}

and compiler pops the error code off stack before the 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.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 43432.Dec 21 2015, 10:20 PM
ABataev retitled this revision from to [X86] Support 'interrupt' attribute for x86.
ABataev updated this object.
ABataev added reviewers: aaron.ballman, rjmccall.
ABataev updated this object.
ABataev updated this object.
ABataev updated this object.
ABataev added a subscriber: cfe-commits.
ABataev updated this object.Dec 21 2015, 10:22 PM
ABataev updated this object.
aaron.ballman added inline comments.Dec 27 2015, 10:46 AM
include/clang/Basic/AttrDocs.td
1867 ↗(On Diff #43432)

Should we also explicitly list x86-64 as well?

1889 ↗(On Diff #43432)

"the pointer pointing to"

What do you mean by "properly define"? Do you mean it cannot be an opaque pointer, or that there is a particular structure that must be used?

include/clang/Basic/DiagnosticSemaKinds.td
2494 ↗(On Diff #43432)

It would be good to model these new diagnostics after the MIPS interrupt diagnostics.

def warn_mips_interrupt_attribute : Warning<
   "MIPS 'interrupt' attribute only applies to functions that have "
   "%select{no parameters|a 'void' return type}0">,
   InGroup<IgnoredAttributes>;
2501 ↗(On Diff #43432)

"can't be used directly": what does it mean to "use"? Take the address of? Call? Pass as an argument to another function call?

lib/CodeGen/TargetInfo.cpp
2013 ↗(On Diff #43432)

Is there a way to prevent this code duplication?

lib/Sema/SemaDeclAttr.cpp
4555 ↗(On Diff #43432)

Based on this, the Subject line in Attr.td should be HasFunctionProto.

4556 ↗(On Diff #43432)

This means it is acceptable to have:

namespace Foo {
struct interrupt_frame;

__attribute__ ((interrupt))
void f (struct interrupt_frame *frame) {
}
}

But not okay to have:

struct interrupt_frame;
class Foo {
  __attribute__ ((interrupt))
  static void f (struct interrupt_frame *frame) {
  }
}

Is that expected? If you want to disallow namespaces, I think you want isTranslationUnit() instead of isFileContext().

4558 ↗(On Diff #43432)

This should be using ExpectedFunctionWithProtoType instead.

4568 ↗(On Diff #43432)

Please do not use auto here.

4579 ↗(On Diff #43432)

second argument, if present, must be an unsigned integer.

4581 ↗(On Diff #43432)

This allows types like bool or __uint128_t; is that permissible?

rjmccall added inline comments.Dec 27 2015, 9:35 PM
include/clang/Basic/Attr.td
255 ↗(On Diff #43432)

As far as I'm aware, we don't use "IA" as an abbreviation for both x86 targets anywhere else, and most people won't recognize it without the -32 or -64 suffix. How about "AnyX86"?

lib/Sema/SemaDeclAttr.cpp
4593 ↗(On Diff #43432)

Please go ahead and refactor this into a switch statement. It's okay to have a default case.

lib/Sema/SemaExpr.cpp
4972 ↗(On Diff #43432)

I think NDecl has to be non-null here.

ABataev marked 8 inline comments as done.Dec 28 2015, 1:19 AM

Aaron, thanks for the review!

include/clang/Basic/AttrDocs.td
1867 ↗(On Diff #43432)

Ok, will do

1889 ↗(On Diff #43432)

I updated the description of this attribute to the latest one. Example also changed

include/clang/Basic/DiagnosticSemaKinds.td
2494 ↗(On Diff #43432)

Ok, will do

2501 ↗(On Diff #43432)

You can't do anything with these functions directly: you can't call it, you can't take an address, you can't pass it as an argument.

lib/Sema/SemaDeclAttr.cpp
4555 ↗(On Diff #43432)

Ok, will do

4556 ↗(On Diff #43432)

Yes, we allow any non-member functions.

4581 ↗(On Diff #43432)

Checked with gcc, made it more strict. Thanks for catching this!

ABataev marked 9 inline comments as done.Dec 28 2015, 1:22 AM

John, thanks for the review!

include/clang/Basic/Attr.td
255 ↗(On Diff #43432)

Ok, I'm fine with it, renamed

lib/Sema/SemaDeclAttr.cpp
4593 ↗(On Diff #43432)

Thought about it, done

lib/Sema/SemaExpr.cpp
4972 ↗(On Diff #43432)

Yes, removed this extra check, thanks!

ABataev updated this revision to Diff 43676.Dec 28 2015, 3:40 AM
ABataev marked 3 inline comments as done.

Update after review

aaron.ballman added inline comments.Dec 28 2015, 8:21 AM
include/clang/Basic/Attr.td
1531 ↗(On Diff #43676)

Maybe this (and IAInterruptDocs) should be named AnyX86Interrupt as well.

lib/Sema/SemaDeclAttr.cpp
4556 ↗(On Diff #43676)

Yes, we allow any non-member functions.

Then we should have a test that shows this working with a named namespace interrupt function. Also, it's a bit odd to me that members of named namespaces are fine, but static member functions are not. This disallows possibly-sensible code (like a private static member function of a class for better encapsulation).

test/Sema/attr-x86-interrupt.c
54 ↗(On Diff #43676)

I'd like to see a test like:

__attribute__((interrupt)) void foo8(int *a) {}

void g(void (*fp)(int *));

void f() {
  g(foo8);
}

as I think this is supposed to diagnose. It might also be good to have a similar test using templates in C++.

ABataev marked 2 inline comments as done.Dec 29 2015, 2:48 AM
ABataev added inline comments.
test/Sema/attr-x86-interrupt.c
54 ↗(On Diff #43676)

Checked again. I did not look at the code for a long time and just forgot that the functions cannot be called, but you can take their address. Modified the message and the code itself for better check.

ABataev updated this revision to Diff 43727.Dec 29 2015, 2:49 AM

Update after review

aaron.ballman added inline comments.Dec 30 2015, 6:25 AM
include/clang/Basic/DiagnosticSemaKinds.td
2504 ↗(On Diff #43727)

It would be good to model these new diagnostics after the MIPS interrupt diagnostics.

def warn_mips_interrupt_attribute : Warning<

"MIPS 'interrupt' attribute only applies to functions that have "
"%select{no parameters|a 'void' return type}0">,
InGroup<IgnoredAttributes>;

Ok, will do

This does not appear to have been completed yet.

lib/Sema/SemaDeclAttr.cpp
4556 ↗(On Diff #43727)

Yes, we allow any non-member functions.

Then we should have a test that shows this working with a named namespace interrupt function. Also, it's a bit odd to me that members of named namespaces are fine, but static member functions are not. This disallows possibly-sensible code (like a private static member function of a class for better encapsulation).

I don't see a test using a named namespace with an interrupt function (though I do see one disallowing a static member function). Also, I am still wondering why there is a restriction against static member functions. (I looked at GCC's docs and they do not claim to support this attribute for these targets, so I'm not certain what this design is based on.)

test/SemaCXX/attr-x86-interrupt.cpp
51 ↗(On Diff #43727)

Ah, I'm sorry, I wasn't very clear with my template request. I was looking for something like (in addition to what you've added):

template <typename Fn>
void bar(Fn F) {
  F(nullptr); // Should this diagnose? I expect not.
}

__attribute__((interrupt)) void foo(int *) {}
void f() {
  bar(foo);
}

Aaron, thanks for the review!

include/clang/Basic/DiagnosticSemaKinds.td
2504 ↗(On Diff #43727)

Probably I did not quite understood your comment. I'll try to fix it and hope this time I'll get it right. :)

lib/Sema/SemaDeclAttr.cpp
4556 ↗(On Diff #43727)

See test/CodeGenCXX/attr-x86-interrupt.cpp. Function 'foo8' is declare in 'namespace S'.

test/SemaCXX/attr-x86-interrupt.cpp
51 ↗(On Diff #43727)

Ok, I will add this test case.

ABataev updated this revision to Diff 43828.Dec 31 2015, 2:01 AM

Fix after review.
After some discussion with gcc guys it was decided to allow to use static member functions as interrupt handler.

aaron.ballman added inline comments.Dec 31 2015, 7:19 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43828)

interrupt service routine -> %select{x86|x86-64}0 'interrupt' attribute only applies to functions that (then fix the other wordings accordingly)

have a void return value -> have a 'void' return type

262 ↗(On Diff #43828)

I would split "can't be called directly" into its own diagnostic. The other diagnostic is about the semantic requirements of the attribute, this is a separate concept. Also, "can't be called directly" should be "cannot be called directly".

lib/Sema/SemaDeclAttr.cpp
4556 ↗(On Diff #43828)

See test/CodeGenCXX/attr-x86-interrupt.cpp. Function 'foo8' is declare in 'namespace S'.

Ah, I was looking for that in the Sema tests because it's a semantic requirement, not a codegen difference. Either place is reasonable, however. Thank you for pointing it out!

ABataev updated this revision to Diff 43843.Jan 1 2016, 11:36 PM

Update after review

aaron.ballman added inline comments.Jan 2 2016, 6:22 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43843)

I still dislike "interrupt service routine" -- there is no interrupt service routine involved here because the attribute does not apply to that function to make it one. The issue is with the attribute's requirements on the function type, so I still prefer "x86 |x86-64'interrupt' attribute only applies to functions that...". Also, "have 'void' return value" is still incorrect -- they need to have a void return type. Something like:

"%select{x86|x86-64}0 'interrupt' attribute only applies to functions that have %select{a 'void' return type|only a pointer parameter optionally followed by an integer parameter|a pointer as the first parameter|a %2 type as the second parameter}1"

(I also changed argument to parameter because arguments are on the caller side, and parameters are on the function declaration side.)

264 ↗(On Diff #43843)

can't -> cannot

rjmccall added inline comments.Jan 3 2016, 8:54 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43843)

It's fine to call it an interrupt service routine because the user intent to declare it as one is obvious. It's still an ISR, it's just an ill-formed ISR. It would even make some amount of sense to actually apply the attribute in the AST and just mark the declaration ill-formed.

aaron.ballman added inline comments.Jan 3 2016, 9:13 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43843)

That would make this diagnostic inconsistent with the other interrupt attribute diagnostics used for other targets, and I don't think it adds any value to do so. I don't think it makes sense to have the attribute mark the declaration as ill-formed either -- attributes are usually side information to the declaration (otherwise we implement them as keywords).

rjmccall added inline comments.Jan 3 2016, 9:17 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43843)

If it's more consistent to use the other wording, that's fine. This attribute is quite definitely not side information to the declaration, however, as it significantly changes its language and ABI rules; and that's true of many other attributes. Attributes are very frequently used in lieu of keywords as an open-ended language extension mechanism.

aaron.ballman added inline comments.Jan 3 2016, 9:26 AM
include/clang/Basic/DiagnosticSemaKinds.td
259 ↗(On Diff #43843)

That's true, we tend to be more lax with attributes than the standards committees are. My biggest concern is with consistency, but I don't really care which direction we go. It fits my mental model slightly better to complain about the attribute when the attribute mismatches with the declaration, because the declaration can stand on its own. However, since this is an error and not a warning where we drop the attribute, I can see the logic behind diagnosing that the declaration is wrong. The other attributes are inconsistent in this case. ARM and MIPS warns and drops the attribute, MSP430 errs.

ABataev updated this revision to Diff 44754.Jan 13 2016, 8:03 AM

Update after review

aaron.ballman accepted this revision.Jan 14 2016, 6:11 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Jan 14 2016, 6:11 AM
This revision was automatically updated to reflect the committed changes.