[cfe-commits] [PATCH] Improved __declspec support

[Aaron Ballman]

On Sat, Jun 16, 2012 at 2:46 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, Jun 14, 2012 at 7:35 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Wed, Jun 13, 2012 at 7:17 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>>> -/// ParseMicrosoftDeclSpec - Parse an __declspec construct
>>> +  ExprResult ArgExpr(ParseAssignmentExpression());
>>>
>>> Should this be parsed in an Unevaluated or ConstantEvaluated context?
>>
>> That's a good question -- can you give me a brief primer on the
>> difference?  I didn't modify the expression argument parsing code from
>> its original state, so I don't have a good answer for you.
>
> The documentation from Sema.h is reasonable:
>
>  /// \brief Describes how the expressions currently being parsed are
>  /// evaluated at run-time, if at all.
>  enum ExpressionEvaluationContext {
>    /// \brief The current expression and its subexpressions occur within an
>    /// unevaluated operand (C++11 [expr]p7), such as the subexpression of
>    /// \c sizeof, where the type of the expression may be significant but
>    /// no code will be generated to evaluate the value of the expression at
>    /// run time.
>    Unevaluated,
>
>    /// \brief The current context is "potentially evaluated" in C++11 terms,
>    /// but the expression is evaluated at compile-time (like the values of
>    /// cases in a switch statment).
>    ConstantEvaluated,
>
>    /// \brief The current expression is potentially evaluated at run time,
>    /// which means that code may be generated to evaluate the value of the
>    /// expression at run time.
>    PotentiallyEvaluated,
>
> The align attribute should be ConstantEvaluated. I'm not sure what the
> other attributes need, but perhaps that will work for all of them.

Thank you for the primer -- I've changed it so that they're all parsed
as constant expressions (turns out there's already a call for this as
well).

>>> Your test changes don't seem to cover any of these. Do we already have tests
>>> for these cases?
>>
>> We have test cases for most of those, but not all -- we don't handle
>> all of them.  This brings up a neat problem I ran into with regards to
>> diagnostic reporting.  I believe there should be a "this is a valid
>> declspec but we are ignoring it" warning as part of the semantic
>> analysis, and a "this isn't a valid declspec at all" as part of the
>> parsing.  I've got the latter, but the former doesn't exist yet
>> because of a problem I ran into.  ProcessDeclAttribute in
>> SemaDeclAttr.cpp seems like it would be the right place to emit the
>> semantic warning, since that's where we determine whether it's a known
>> attribute or not.  However, it gets called multiple times for the same
>> declaration (see ProcessdeclAttributes), so the warning would fire
>> multiple times.
>
> Each call to ProcessDeclAttributeList has a different Attrs argument,
> so this may actually be OK. Issuing a diagnostic from within
> ProcessDeclAttribute is what we do for inheritable attributes (see the
> end of ProcessInheritableDeclAttr).

This took a bit more work than you might think.  The big problem was
that type attributes are processed as regular attributes, so things
like Borland's _cdecl are attached as unknown attributes.  So to solve
this, I took care of the FIXME in ParseDecl by attaching information
to the AttributeList as to whether it's a type attribute or not.  Then
when doing the semantic processing, I check for type attributes and
ignore them entirely.  This way, we warn properly for unknown
attributes, known but unhandled Microsoft declspecs, error on unknown
Microsoft declspecs, and do not diagnose type attributes improperly.

>>> +    // The parser should catch this and never create the attribute in the
>>> first
>>> +    // place.
>>> +    assert(Attr.getName()->getName() == "align" &&
>>> +            "Parser did not catch align attribute spelling");
>>> +  }
>>>
>>> An assertion seems like overkill for this -- a testcase would seem
>>> sufficient.
>>
>> How would you frame the testcase?  Would the one in
>> Sema\MicrosoftCompatibility.c be sufficient (at which point I just
>> remove the assert)?
>
> Yes, that seems fine to me.

Removed the assert.

>>> +  if (Context.getLangOpts().MicrosoftMode) {
>>> +    // We've already verified it's a power of 2, now let's make sure it's
>>> +    // 8192 or less
>>> +    if (Alignment.getZExtValue() > 8192) {
>>> +      Diag(AttrLoc, diag::err_attribute_aligned_greater_than_8192)
>>> +        << E->getSourceRange();
>>> +      return;
>>> +    }
>>> +  }
>>>
>>> Should this be checking Attr.isDeclspecAttribute() rather than
>>> MicrosoftMode?
>>
>> Ideally, yes.  But the problem is that the information isn't always
>> known by that point.  InstantiateAttrs in
>> SemaTemplateInstantiateDecl.cpp has a call to AddAlignedAttr that
>> doesn't have access to the AttributeList so it doesn't know whether it
>> came from a declspec or not.  I suspect I could assume it doesn't, but
>> I don't know for sure.  The alternative would be to teach Attr about
>> declspecs since it seems to have some room in its bitfields.
>
> I like (something like) the alternative option. Since the semantics of
> this attribute (at least, its semantics under template instantiation)
> depend on whether it's a __declspec attribute or not, we should be
> able to work that out from the AlignedAttr. For AST printing,
> refactoring, and source fidelity in general, it would be useful to
> store enough information on the AST to recover what syntax was used
> and the source extent of the surrounding attribute declaration, but
> the design and discussion of such a change should be kept separate
> from this patch. Perhaps for now we should just store a flag on the
> AlignedAttr to indicate if it's an MS align attribute with an 8192
> limit.

I added the information to the AlignedAttr directly and am using it as
appropriate.  I think we could eventually hoist this information up
into Attr instead of leaving it on AlignedAttr, but you are correct
that it should be a separate patch and discussion.

I've attached my revised patch for another round of review.  Thank you
for all the thoughtful reviews!

~Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Declspecs.patch
Type: application/octet-stream
Size: 29831 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120617/899843c3/attachment.obj>