[PATCH] ARM: Implement __attribute__((interrupt(...)))

[Aaron Ballman]

On Tue, Sep 24, 2013 at 8:20 AM, Tim Northover <t.p.northover at gmail.com> wrote:
> Hi Aaron,
>
> Thanks for your review, I'm working on an updated Clang patch. Could I
> ask a couple of questions?
>
>>> +def err_attribute_argument_invalid : Error<
>>> +  "'%0' attribute parameter '%1' is invalid">;
>>
>> This is not needed (more below).
>> [...]
>> There is code auto-generated for you that does this, named something like
>> ConvertStrToInterruptType; you should use that, and get rid of the custom
>> logic for error handling.
>
> As far as I can see, the auto-generated logic is very self-contained
> and only takes a StringRef and converts it to an enum. Am I missing a
> separate interface that'll do the whole Sema for me? Otherwise it
> seems most of the error-handling is necessary.

Nope, I've not had the chance to complete all of the sema
automagically yet.  The auto-generated logic handles converting the
StringRef to an enum, and tells you whether the conversion happened or
not -- so you can remove the StringSwitch part, but keep the
diagnostic.  However, the usual diagnostic is
warn_attribute_type_not_supported, which is why you don't need the new
one added.  At some point, I will get around to pushing more of the
sema logic into the auto-generated code, so having this be consistent
across attributes is important for my future sanity.

>
>>> +    Fn->addFnAttr("interrupt", Kind);
>>> +
>>> +    if (cast<ARMABIInfo>(getABIInfo()).getABIKind() == ARMABIInfo::APCS)
>>> +      return;
>>
>> This could be hoisted higher in the function.
>
> Any higher and the LLVM function would not get the attribute.

Ah, good point!  The addFnAttr slipped past my eyes.

>
>>> +  d->addAttr(::new (S.Context)
>>> +             ARMInterruptAttr(Attr.getLoc(), S.Context,
>>> Kind.getValue()));
>>> +  d->addAttr(::new (S.Context) UsedAttr(Attr.getLoc(), S.Context));
>>
>> This breaks the as-written attributes.
>
> Do you mean the fact that we end up with two "used" attributes if it's
> also specified manually? Or am I doing something wrong with the
> ARMInterruptAttr too (aside from not setting the spelling index)?

No, I mean that anyone using the interrupt attribute will
automatically get a used attribute, even though they did not actually
write one.  This is an unfortunate problem since we can't reliably
regenerate the user's original source, but it's not unique to your
code (it happens in a few other places as well).  If we can possibly
get rid of the UsedAttr addition, that would be fantastic.  If not,
it'll be a problem we have to solve another day, but it is unfortunate
that we're compounding the issue.

~Aaron