[cfe-commits] [PATCH] Improved __declspec support

[Aaron Ballman]

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.

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

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

> +  IdentifierInfo *AttrName = Tok.getIdentifierInfo();
> +  SourceLocation AttrNameLoc = ConsumeToken();
> +
> +  if (IsString || IsSimpleMicrosoftDeclSpec(AttrName)) {
> +    // If we have a generic string, we will allow it because there is no
> +    // documented list of allowable string declspecs, but we know they
> exist (for
> +    // instance, SAL declspecs in older versions of MSVC).
> +    //
> +    // Alternatively, if the identifier is a simple one, then it requires
> no
> +    // arguments and can be turned into an attribute directly.
> +    Attrs.addNew(AttrName, AttrNameLoc, 0, AttrNameLoc, 0,
> SourceLocation(),
> +                  0, 0, true );
>
> I would expect AttrName to be null in the IsString case. Is it useful to add
> such an attribute?

No, that's a bug.

> +    // 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)?

> +  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'll work on your other suggestions in the meantime.  Thank you for
the thoughtful review!

~Aaron