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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/AST/Type.cpp:382-386
+QualType QualType::IgnoreMacroDefinitions(QualType T) {
+  while (const auto *MDT = T->getAs<MacroDefinedType>())
+    T = MDT->getUnderlyingType();
+  return T;
+}
----------------
rsmith wrote:
> This doesn't seem like a sufficiently general operation to warrant being a member of `QualType` to me, and like `IgnoreParens` before it, this is wrong with regard to qualifiers (it drops top-level qualifiers on the type if they're outside the macro qualifier). Looking through the uses below, I think there are better ways to write them that avoid the need for this function.
Removed this method.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:257-262
+      bool AttrStartIsInMacro =
+          (StartLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion(
+                                       StartLoc, SrcMgr, PP.getLangOpts()));
+      bool AttrEndIsInMacro =
+          (EndLoc.isMacroID() &&
+           Lexer::isAtEndOfMacroExpansion(EndLoc, SrcMgr, PP.getLangOpts()));
----------------
rsmith wrote:
> I think these checks need to be moved to the last loop of `FindLocsWithCommonFileID`. Otherwise for a case like:
> 
> ```
> #define THING \
>   int *p = nullptr;
>   AS1 int *q = nullptr;
> 
> THING
> ```
> 
> ... `FindLocsWithCommonFileID` will pick out the `THING` macro and return the range from the `int` token to the second semicolon, and the checks here will fail. Instead, we should pick out the inner `AS1` expansion, because it's the outermost macro expansion that starts with `StartLoc` and ends with `EndLoc`.
Moved, although this doesn't appear to fix this case. On closer inspection, when generating the vector of starting locations, the expansion location for `AS1` doesn't seem to be returned by `getExpansionLocStart`. It goes straight from the location of the `__attribute__` behind `AS1` to `THING` and skips `AS1`. I found this out by just dumping `StartLoc` on every iteration.

The only way I can generate the location for `AS1` in `THING` is by also watching for the spelling location of each expansion, but this SourceLoc is not a macro ID and would not fulfill these last 2 checks. Is this intended? If it's a bug I don't mind addressing it if you point me in the right direction to start.


================
Comment at: clang/lib/Sema/SemaType.cpp:6967-6972
+  QualType R = T.IgnoreParens().IgnoreMacroDefinitions();
   while (const AttributedType *AT = dyn_cast<AttributedType>(R)) {
     if (AT->isCallingConv())
       return true;
-    R = AT->getModifiedType().IgnoreParens();
+    R = AT->getModifiedType().IgnoreParens().IgnoreMacroDefinitions();
   }
----------------
rsmith wrote:
> Instead of calling `IgnoreParens()` and `IgnoreMacroDefinitions` here, I think we should just be ignoring any type sugar by using `getAs<AttributedType>()`:
> 
> ```
> bool Sema::hasExplicitCallingConv(QualType T) {
>   while (const auto *AT = T->getAs<AttributedType>()) {
>     if (AT->isCallingConv())
>       return true;
>     T = AT->getModifiedType();
>   }
>   return false;
> }
> ```
> 
> (Note also the change from passing `T` by reference -- and then never storing to it in the function -- to passing it by value.)
When using only `getAs`, the following 3 tests fail:

    Clang :: CodeGenCXX/mangle-ms.cpp
    Clang :: SemaCXX/calling-conv-compat.cpp
    Clang :: SemaCXX/decl-microsoft-call-conv.cpp

since using `getAs` would involved removing a `TypeDefType` for each of these. Looking at the comment above this method's declaration, part of the intent is to retain the typedef sugar. We could still use `getAs` and not `IgnoreMacroDefinitions` by additionally checking for a typedef:

```
const AttributedType *AT;
while ((AT = T->getAs<AttributedType>()) && !isa<TypedefType>(T)) {
```

Using `!T->getAs<TypedefType>()` will still fail for types laid out as:

```
AttributedType
` - TypedefType
```


================
Comment at: clang/lib/Sema/SemaType.cpp:7553-7560
+    // Handle attributes that are defined in a macro.
+    if (attr.hasMacroIdentifier() && !type.getQualifiers().hasObjCLifetime()) {
+      const IdentifierInfo *MacroII = attr.getMacroIdentifier();
+      if (FoundMacros.find(MacroII->getName()) == FoundMacros.end()) {
+        type = state.getSema().Context.getMacroDefinedType(type, MacroII);
+        FoundMacros.insert(MacroII->getName());
+      }
----------------
rsmith wrote:
> The handling (both here and in the parser) for a macro that defines multiple attributes doesn't seem right. Given:
> 
> ```
> #define ATTRS __attribute__((attr1, attr2))
> ATTRS int x;
> ```
> 
> it looks like you'll end up with a type built as
> 
> ```
> AttributedType attr1
>  `- MacroDefinedType
>       `- AttributedType attr2
>           `- int
> ```
> 
> ... which seems wrong.
> 
> 
> The parser representation also can't distinguish between the above case and
> 
> ```
> #define ATTRS __attribute__((attr1))
> ATTRS
> #define ATTRS __attribute__((attr2))
> ATTRS
> int x;
> ```
> 
> ... since both will be represented as simply a macro identifier `ATTR` attached to two different attributes (you could solve this by tracking the source location of the expansion loc of the macro in the parsed attribute representation, instead of or in addition to the macro identifier).
> 
> And the AST representation can't distinguish between the above and
> 
> ```
> #define ATTRS __attribute__((attr1))
> ATTRS __attribute__((attr2)) int x;
> ```
> 
> ... because it doesn't track what the modified type was, so we have to guess which attributes are covered by the macro.
> 
> Perhaps the best thing would be to support only the case of a macro that expands to exactly one attribute for the initial commit, and add support for macros covering multiple attributes as a follow-up commit?
If it makes this cleaner and easier to review, I don't mind separating it into 2 patches with the first working for exactly one and the followup for multiple. For us though, we would mainly be using this for multiple attribute macros (especially `__attribute__((address_space(n), noderef))`. I can split it into 2 if you would like after this revision.

As for the cases you presented, for

```
#define ATTRS __attribute__((attr1, attr2))
ATTRS int x;
```

I see that this prevents from adding any more `MacroQualifiedType`s for attributes that share the same name. Updated in this revision so that we continue to wrap `AttributedType`s, producing:

```
MacroQualifiedType (ATTRS)
 `- AttributedType attr1
     ` - MacroQualifiedType (ATTRS)
        `- AttributedType attr2
             `- int
```

In the case of 

```
#define ATTRS __attribute__((attr1))
ATTRS
#define ATTRS __attribute__((attr2))
ATTRS
int x;
```

I added the expansion loc so we can distinguish between this and the first case now.

As for

```
#define ATTRS __attribute__((attr1))
ATTRS __attribute__((attr2)) int x;
```

With this new revision, the AST representation now differs from the previous 2 examples in that only `attr1` gets wrapped with a `MacroQualifiedType` and not `attr2` producing:

```
AttributedType attr2
 ` - MacroQualifiedType (ATTRS)
    `- AttributedType attr1
         `- int
```

I also added test cases for these.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51329/new/

https://reviews.llvm.org/D51329