[cfe-commits] [PATCH] Improved __declspec support
Richard Smith
richard at metafoo.co.uk
Sun Jun 17 16:07:14 PDT 2012
On Sun, Jun 17, 2012 at 10:09 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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!
Mostly just nitpicking:
> Index: include/clang/Basic/DiagnosticParseKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticParseKinds.td (revision 158272)
> +++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
> @@ -481,6 +481,9 @@
> "introducing an attribute">;
> def err_alignas_pack_exp_unsupported : Error<
> "pack expansions in alignment specifiers are not supported yet">;
> +def err_unknown_ms_declspec : Error<"unrecognized __declspec attribute %0">;
This should probably be a Warning, and InGroup<UnknownAttributes>. It
would also seems sensible to mirror the wording of
warn_unknown_attribute_ignored: "unknown __declspec attribute %0
ignored".
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 158272)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -1619,11 +1619,16 @@
>
> def err_attribute_aligned_not_power_of_two : Error<
> "requested alignment is not a power of 2">;
> +def err_attribute_aligned_greater_than_8192 : Error<
> + "requested alignment must be 8192 bytes or smaller">;
> def warn_redeclaration_without_attribute_prev_attribute_ignored : Warning<
> "'%0' redeclared without %1 attribute: previous %1 ignored">;
> def warn_attribute_ignored : Warning<"%0 attribute ignored">;
> def warn_unknown_attribute_ignored : Warning<
> "unknown attribute %0 ignored">, InGroup<UnknownAttributes>;
> +def warn_unhandled_ms_attribute_ignored : Warning<
> + "__declspec %0 is a valid Microsoft attribute, but is ignored">,
> + InGroup<IgnoredAttributes>;
Perhaps "__declspec attribute %0 is not supported"?
> Index: include/clang/Parse/Parser.h
> ===================================================================
> --- include/clang/Parse/Parser.h (revision 158272)
> +++ include/clang/Parse/Parser.h (working copy)
> @@ -1790,7 +1790,14 @@
> }
> void ParseMicrosoftAttributes(ParsedAttributes &attrs,
> SourceLocation *endLoc = 0);
> - void ParseMicrosoftDeclSpec(ParsedAttributes &attrs);
> + void ParseMicrosoftDeclSpec(ParsedAttributes &Attrs);
> + bool IsSimpleMicrosoftDeclSpec(IdentifierInfo *Ident);
> + bool ParseComplexMicrosoftDeclSpec(IdentifierInfo *Ident,
> + SourceLocation Loc,
> + ParsedAttributes &Attrs);
> + void ParseMicrosoftDeclSpecWithSingleArg(IdentifierInfo *AttrName,
> + SourceLocation AttrNameLoc,
> + ParsedAttributes &Attrs);
There's one too many spaces in the indentation for the wrapped
arguments in both of these.
> Index: include/clang/Sema/AttributeList.h
> ===================================================================
> --- include/clang/Sema/AttributeList.h (revision 158272)
> +++ include/clang/Sema/AttributeList.h (working copy)
> @@ -80,6 +80,9 @@
> /// availability attribute.
> unsigned IsAvailability : 1;
>
> + /// True if this attribute is actually a type attribute
> + unsigned IsTypeAttribute : 1;
Sean Hunt has a patch for this class which will replace the
DeclspecAttribute and CXX0XAttribute flags with an enum for the
syntactic form used for the attribute. It would make sense to add this
as a value in the enum, instead of adding another flag (so you're
going to need to wait a short while for Sean's patch to land), since
these attributes don't use the syntactic form of __declspec
attributes. The comment (and possibly the name) could also be improved
to indicate that these are Microsoft type attribute keywords (and give
a couple of examples in the comment).
> Index: lib/Parse/ParseDecl.cpp
> ===================================================================
> --- lib/Parse/ParseDecl.cpp (revision 158272)
> +++ lib/Parse/ParseDecl.cpp (working copy)
> @@ -281,61 +281,164 @@
> }
> }
>
> +///
> +/// ParseMicrosoftDeclSpecWithSingleArg
> +///
Drop this part of the comment for all these functions.
> +/// \brief Parses a single argument for a declspec, including the
> +// surrounding parens
This // should be a ///, and you're missing a full stop.
> +void Parser::ParseMicrosoftDeclSpecWithSingleArg(IdentifierInfo *AttrName,
> + SourceLocation AttrNameLoc,
> + ParsedAttributes &Attrs)
One too many spaces in the indentation of these parameters.
> +{
> + BalancedDelimiterTracker T(*this, tok::l_paren);
> + if (T.expectAndConsume(diag::err_expected_lparen_after,
> + AttrName->getNameStart(), tok::r_paren)) {
> + return;
> + }
Drop these braces.
>
> -/// ParseMicrosoftDeclSpec - Parse an __declspec construct
> + ExprResult ArgExpr(ParseConstantExpression());
> + if (!ArgExpr.isInvalid()) {
> + Expr *ExprList = ArgExpr.take();
> + Attrs.addNew(AttrName, AttrNameLoc, 0, AttrNameLoc, 0, SourceLocation(),
> + &ExprList, 1, true);
One too many spaces in indentation here.
> + } else {
> + T.skipToEnd();
> + }
> +
> + T.consumeClose();
skipToEnd consumes the closing paren, so you can change your error
handling code to:
if (ArgExpr.isInvalid()) {
T.skipToEnd();
return;
}
A test for this error path would be useful.
> +/// \brief Attempts to parse a declspec which is not simple (one that takes
> +/// parameters). Will return false if we properly handled the declspec, or
> +/// true if it is an unknown declspec.
> +bool Parser::ParseComplexMicrosoftDeclSpec(IdentifierInfo *Ident,
> + SourceLocation Loc,
> + ParsedAttributes &Attrs) {
One space too many in indentation of these parameters.
> + // Try to handle the easy case first -- these declspecs all take a single
> + // parameter as their argument
Missing full stop.
> + if (llvm::StringSwitch<bool>(Ident->getName())
> + .Case("uuid", true)
> + .Case("align", true)
> + .Case("allocate", true)
> + .Default(false)) {
> + ParseMicrosoftDeclSpecWithSingleArg(Ident, Loc, Attrs);
> + } else if (Ident->getName() == "deprecated") {
> + // The deprecated declspec has an optional single argument, so we will
> + // check for a l-paren to decide whether we should parse an argument or
> + // not.
> + if (Tok.getKind() == tok::l_paren)
> + ParseMicrosoftDeclSpecWithSingleArg(Ident, Loc, Attrs);
> + else
> + Attrs.addNew(Ident, Loc, 0, Loc, 0, SourceLocation(), 0, 0, true );
Remove the extra space at the end of the argument list.
> + } else if (Ident->getName() == "property") {
> + // The property declspec is more complex in that it can take one or two
> + // assignment expressions as a parameter, but the lhs of the assignment
> + // must be named get or put.
> + //
> + // For right now, we will just skip to the closing right paren of the
> + // property expression
Missing full stop.
> -void Parser::ParseMicrosoftDeclSpec(ParsedAttributes &attrs) {
> +void Parser::ParseMicrosoftDeclSpec(ParsedAttributes &Attrs) {
A general comment about this function: the MS documentation suggests
that multiple attributes may be specified in the same __declspec, for
instance "__declspec(dllexport naked)". It appears that the old code
supported this and the new code doesn't. Is that intentional?
> assert(Tok.is(tok::kw___declspec) && "Not a declspec!");
>
> ConsumeToken();
> - if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after,
> - "declspec")) {
> - SkipUntil(tok::r_paren, true); // skip until ) or ;
> + BalancedDelimiterTracker T(*this, tok::l_paren);
> + if (T.expectAndConsume(diag::err_expected_lparen_after, "__declspec",
> + tok::r_paren)) {
One too many spaces here.
> return;
> }
Remove these braces.
> + // We expect either a well-known identifier or a generic string. Anything
> + // else is a malformed declspec.
> + bool IsString = Tok.getKind() == tok::string_literal ? true : false;
bool IsString = Tok.getKind() == tok::string_literal;
> + IdentifierInfo *AttrName;
> + SourceLocation AttrNameLoc;
> + if (IsString) {
> + SmallString<8> StrBuffer;
> + bool Invalid = false;
> + StringRef Str = PP.getSpelling(Tok, StrBuffer, &Invalid);
> + if (Invalid) {
> + T.skipToEnd();
> + return;
> }
> + AttrName = PP.getIdentifierInfo( Str );
No spaces around Str here, please. Building an IdentifierInfo for
something which isn't lexcially an identifier is unfortunate, but it
seems that's forced upon us by AttributeList.
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp (revision 158272)
> +++ lib/Sema/SemaDeclAttr.cpp (working copy)
> @@ -4162,8 +4164,11 @@
> if (Attr.isInvalid())
> return;
>
> - if (Attr.isDeclspecAttribute() && !isKnownDeclSpecAttr(Attr))
> - // FIXME: Try to deal with other __declspec attributes!
> + // Type attributes are still treated as declaration attributes by
> + // ParseMicrosoftTypeAttributes and ParseBorlandTypeAttributes. We don't
> + // want to process them, however, because we will simply warn about ignoring
> + // them. So instead, we will bail out early.
> + if (Attr.isTypeAttribute())
> return;
Can you check Attr.isUsedAsTypeAttr() instead, in the code path which
produces the warning? This would presumably allow the checks for
AT_address_space, AT_opencl_image_access etc in
ProcessInheritableDeclAttr to be removed too.
Thanks!
Richard
More information about the cfe-commits
mailing list