[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 29 16:06:48 PDT 2019
rsmith added inline comments.
Herald added a subscriber: rnkovacs.
================
Comment at: clang/include/clang/AST/Type.h:4174
+/// Sugar type that represents a type that was defined in a macro.
+class MacroQualifiedType : public Type {
----------------
This comment is out of date. "[...] a type that was qualified by a qualifier written as a macro invocation" maybe?
================
Comment at: clang/include/clang/AST/Type.h:4183
+ MacroQualifiedType(QualType UnderlyingTy, QualType CanonTy,
+ const IdentifierInfo *MacroII, SourceLocation ExpansionLoc)
+ : Type(MacroQualified, CanonTy, UnderlyingTy->isDependentType(),
----------------
Types should not carry source locations. If you want location information, it should be stored on the `TypeLoc` instead.
================
Comment at: clang/include/clang/AST/Type.h:4199-4201
+ /// Return this attributed type's modified type with no qualifiers attached to
+ /// it.
+ QualType getUnqualifiedType() const;
----------------
`getUnqualifiedType` means something very specific in Clang, and this isn't it. `getModifiedType` would be a better name.
================
Comment at: clang/include/clang/AST/TypeLoc.h:1086
+struct MacroQualifiedLocInfo {};
+
----------------
This struct is where the `ExpansionLoc` should live.
================
Comment at: clang/lib/AST/TypePrinter.cpp:107-108
+ // Set for preventing printing of macros with the same name.
+ llvm::StringSet<> PrintedMacros;
+
----------------
I think this is more expensive than we need and unnecessary... see below.
================
Comment at: clang/lib/AST/TypePrinter.cpp:982
+ // we trim any attributes and go directly to the original modified type.
+ printBefore(T->getUnqualifiedType(), OS);
+
----------------
Instead of recursing to the modified type here, and using a separate set to prevent printing the same macro twice, how about something like:
```
// Step over MacroQualifiedTypes from the same macro to find the type ultimately
// qualified by the macro qualifier.
QualType Inner = T->getModifiedType();
while (auto *InnerMQT = dyn_cast<MacroQualifiedType>(Inner)) {
if (InnerMQT->getMacroIdentifier() != T->getMacroIdentifier())
break;
Inner = InnerMQT->getModifiedType();
}
printBefore(Inner, OS);
```
(and likewise in `printAfter`). And perhaps the above loop should be in `MacroQualifiedType::getModifiedType()` rather than here.
================
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()));
----------------
leonardchan wrote:
> 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.
Right, sorry, mistake on my part. To deal with this, you need to call `isAt{Start,End}OfMacroExpansion` while collecting `StartLocs` and `EndLocs`, and stop once you hit a location where they return `false`. (That would mean that you never step from the location of `AS1` out to the location of `THING`.)
================
Comment at: clang/lib/Sema/SemaType.cpp:7373
+ llvm::StringMap<llvm::SmallVector<const IdentifierInfo *, 2>> FoundMacros;
state.setParsedNoDeref(false);
----------------
(Unused, please remove.)
================
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();
}
----------------
leonardchan wrote:
> 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
> ```
Can you give an example of the failures? The approach you're using here isn't quite right, as it will skip over a `typedef` that's wrapped in another type sugar node (when `T` is not a typedef but desugars to one). Something like `&& AT->getAs<TypedefType>() == T->getAs<TypedefType>()` (that is: if we'd be stripping off a `typedef` sugar node to reach the `AttributedType`, then stop) should work, but maybe there's a better way to express that.
================
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());
+ }
----------------
leonardchan wrote:
> 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.
I think the ideal representation for this case would be
```
MacroQualifiedType (ATTRS)
`- AttributedType attr1
`- AttributedType attr2
`- int
```
... but I can see that'll be a pain to produce. Let's go with your current representation for now and look into improving it as a follow-up change. (This'll give weird behavior in some cases: removing the outer `AttributedType` will still lead to a type that pretty-prints as `ATTRS int`, whereas it should pretty-print as `__attribute__((attr2)) int` because `attr1` is not applied to the type.)
================
Comment at: clang/lib/Sema/TreeTransform.h:6184
+
+ TLB.push<MacroQualifiedTypeLoc>(Result);
+ return Result;
----------------
(Copy the expansion loc here.)
================
Comment at: clang/lib/Serialization/ASTReader.cpp:6181
+ case TYPE_MACRO_DEFINED: {
+ if (Record.size() != 3) {
----------------
The name of this enumerator is out of date; should be `TYPE_MACRO_QUALIFIED`
================
Comment at: clang/test/SemaObjC/gc-attributes.m:12-21
+ f0(&a2); // expected-warning{{passing '__weak A **' to parameter of type '__strong A **' discards qualifiers}}
}
void f1(__weak A**); // expected-note{{passing argument to parameter here}}
void test_f1() {
A *a;
----------------
This is wrong: we need to print the macro qualifier on the right in cases like this so that it attaches to the pointer. You can use `TypePrinter::canPrefixQualifier` to determine whether to print the macro name on the left or the right.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51329/new/
https://reviews.llvm.org/D51329
More information about the cfe-commits
mailing list