[cfe-commits] [PATCH] Improved __declspec support

[Richard Smith]

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.

>> +bool Parser::IsSimpleMicrosoftDeclSpec(IdentifierInfo *Ident) {
>> +  return llvm::StringSwitch<bool>(Ident->getName())
>> +    .Case("dllimport", true)
>> +    .Case("dllexport", true)
>> +    .Case("noreturn", true)
>> +    .Case("nothrow", true)
>> +    .Case("noinline", true)
>> +    .Case("naked", true)
>> +    .Case("appdomain", true)
>> +    .Case("process", true)
>> +    .Case("jitintrinsic", true)
>> +    .Case("noalias", true)
>> +    .Case("restrict", true)
>> +    .Case("novtable", true)
>> +    .Case("selectany", true)
>> +    .Case("thread", true)
>> +    .Default(false);
>> +}
>>
>> 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).

> So the code I wrote was assuming the Parser would parse all known
> declspecs, and that Sema would handle reporting on any we don't know
> about.  But I've yet to figure out a reasonable way to do that.  Ideas
> or suggestions are welcome, but I was thinking of punting on the
> problem for the moment and continuing to not warn on known but ignored
> declspecs (it matches the current behavior).

I think all that's missing is a diagnostic for unknown attributes in
ProcessNonInheritableDeclAttr to match the code in
ProcessInheritableDeclAttr. Note, for instance, ToT already produces
this:

<stdin>:1:17: warning: unknown attribute 'dllimport' ignored [-Wattributes]
void __declspec(dllimport) f();
                ^

>> +    // 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.

>> +  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.

- Richard