[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 4 16:51:38 PDT 2018


rsmith added a comment.

I think this patch is nearly ready to commit. However... if you want to handle macros that specify a sequence of attributes, we should switch to treating the macro name as a separate type sugar node rather than modeling it as a name for the `AttributedType` node. Up to you how we proceed from here -- if you want to land this patch with a one-attribute-in-the-macro restriction, that seems fine; if you'd prefer to go straight to a more general approach where we add a new type sugar representation for a single macro describing (potentially) multiple attributes, that's fine too.



================
Comment at: lib/AST/ASTDiagnostic.cpp:78
+            AttributedType::getNullabilityAttrKind(*nullability), RT, RT,
+            FT->getReturnType()->getAs<AttributedType>()->getMacroIdentifier());
       }
----------------
Use `castAs` here and below (because it would be a programming error if the type isn't an `AttributedType`).


================
Comment at: lib/AST/ASTImporter.cpp:991
+      T->getAttrKind(), ToModifiedType, ToEquivalentType,
+      T->getMacroIdentifier());
 }
----------------
You need to import the identifier; the source and destination ASTs have different identifier tables. (`Importer.Import(T->getMacroIdentifier())`).


================
Comment at: lib/AST/TypePrinter.cpp:1370
+
+    // Remove the underlying address space so it won't be printed.
+    SplitQualType SplitTy = T->getModifiedType().split();
----------------
leonardchan wrote:
> rsmith wrote:
> > leonardchan wrote:
> > > rsmith wrote:
> > > > leonardchan wrote:
> > > > > rsmith wrote:
> > > > > > This is unnecessary; just print the modified type here. (The modified type by definition does not have the attribute applied to it.)
> > > > > When you say the modified type, do you mean just the type without it's qualifiers? I wasn't sure if removing all the qualifiers would suffice since there were also other  non-address_space qualifiers that could be printed.
> > > > I mean `T->getModifiedType()`, which tracks what the type was before the attribute was applied.
> > > Oh, I understand that you meant `T->getModifiedType()`. This is a special case when printing the `address_space` since even though the attribute is applied and printed here, when we reach the qualifiers of the modified type, the address space will still be printed unless we remove it here.
> > > 
> > > I'm not sure if there's a better way to do this though.
> > If the address space qualifiers are present on the modified type, then that's a bug in the creation of the `AttributedType`. And indeed looking at r340765, I see we are passing the wrong type in as the modified type when creating the `AttributedType`. Please fix that, and then just use `getModifiedType` here.
> Making another patch for this.
Incidentally, the last two arguments to the `AddressSpaceAttr` constructor in that patch (address space and spelling list index) are also reversed.


================
Comment at: lib/Parse/ParseDecl.cpp:138
   while (Tok.is(tok::kw___attribute)) {
+    Token AttrTok = Tok;
+    unsigned OldNumAttrs = attrs.size();
----------------
No need to copy the entire `Token` here, you only use its location before. It's also idiomatic to fold this into consuming the token:

`SourceLocation AttrTokLoc = ConsumeToken();`


================
Comment at: lib/Parse/ParseDecl.cpp:215-220
+    bool AttrStartIsInMacro =
+        (AttrTokLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion(
+                                       AttrTokLoc, SrcMgr, PP.getLangOpts()));
+    bool AttrEndIsInMacro =
+        (Loc.isMacroID() &&
+         Lexer::isAtEndOfMacroExpansion(Loc, SrcMgr, PP.getLangOpts()));
----------------
You're only looking at the innermost macro expansion; I still think we should take the outermost one. (You can iteratively walk the expansion locations of the start and end locations (`SourceManager::getExpansionLocStart`/`End`) to build a list of `FileID`s representing macro expansions that each of them is included in, and then take the last common such `FileID` that represents an object-like macro. You can use `SourceManager::getSLocEntry` to get the corresponding `ExpansionInfo` for the macro, `isFunctionMacroExpansion` to determine if it's a function-like or object-like macro, and once you know it's object-like, take the spelling locations of the `ExpansionLocStart` and `ExpansionLocEnd` to find the name of the macro.


================
Comment at: lib/Parse/ParseDecl.cpp:244-252
+    // If this was declared in a macro, attatch the macro IdentifierInfo to the
+    // parsed attribute.
+    for (const auto &MacroPair : PP.getAttributeMacros()) {
+      if (SourceLocInSourceRange(AttrTok.getLocation(), MacroPair.first,
+                                 PP.getSourceManager())) {
+        ApplyMacroIIToParsedAttrs(attrs, NumParsedAttrs, MacroPair.second);
+        break;
----------------
leonardchan wrote:
> rsmith wrote:
> > You shouldn't do this if `NumParsedAttrs != 1`. We're only looking for a macro that exactly covers one attribute.
> > 
> > (Also, your computation of the number of attributes in the attribute list is not correct in the presence of late-parsed attributes.)
> One of the things we would like is for this to cover more than one attribute in the attribute list since in sparse, `address_space` is sometimes used with `noderef`.
> 
> So given `# define __user __attribute__((noderef, address_space(1)))`, `__user` would be saved into the `ParsedAttr` made for `noderef` and `address_space`.
> 
> What would be the appropriate way to track newly added attributes into the `ParsedAttributes`, including late-parsed attributes?
> One of the things we would like is for this to cover more than one attribute in the attribute list since in sparse, `address_space` is sometimes used with `noderef`.

Hold on, this is a new requirement compared to what we'd previously discussed (giving a name to an address space). How important is this use case to you?

I don't think it's a reasonable AST model to assign a macro identifier to an `AttributedType` if the macro defines multiple attributes. If you really want to handle that use case, I think that an identifier on the `AttributedType` is the wrong way to model it, and we should instead be creating a new type sugar node representing a type decoration written as a macro.

Assuming you want to go ahead with the current patch direction (at least in the short term), please add the "only one attribute in the macro" check.


Repository:
  rC Clang

https://reviews.llvm.org/D51329





More information about the cfe-commits mailing list