[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration
Leonard Chan via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 30 12:39:12 PDT 2019
leonardchan added inline comments.
================
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:
> 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`.)
Hmm. So I still run into the problem of none of the locations being at the start or end of the macro expansion. In your example, I only find 2 source locations at all. Let's say I have:
```
1 #define AS1 __attribute__((address_space(1)))
2
3 int main() {
4 #define THING \
5 int *p = 0; \
6 AS1 int *q = p;
7
8 THING;
9 }
```
I only see the following expansion source locations:
```
/usr/local/google/home/leonardchan/misc/test.c:8:3 <Spelling=/usr/local/google/home/leonardchan/misc/test.c:1:13>
/usr/local/google/home/leonardchan/misc/test.c:8:3 <Spelling=/usr/local/google/home/leonardchan/misc/test.c:6:3>
```
And it seems none of them return true for `isAtStartOfMacroExpansion` since the `__attribute__` keyword isn't the first token of `THING`. I imagine stopping early would work if the 2nd expansion location instead referred to `AS1` (on line 6 col 3), but it doesn't seem to be the case here.
I can also see similar results for other examples where the macro is nested but does not encapsulate the whole macro:
```
#define THING2 int AS1 *q2 = p2;
int *p2;
THING2; // Error doesn't print AS1
```
For our case, it seems like the correct thing to do is to get the spelling location and default to it if the macro itself doesn't contain the whole attribute. I updated and renamed this function to account for this and we can now see `AS1` printed. Also added test cases for this.
For cases like `#define AS2 AS1`, `AS1` is still printed, but this is intended since `AS1` is more immediate of an expansion than `AS2`.
================
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:
> 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.
Updated.
Also there's just 1 error from each. This can be replicated just by using the new function and omitting the typedef check.
CodeGenCXX/mangle-ms.cpp
```
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-2/clang/test/CodeGenCXX/mangle-ms.cpp:386:15: error: CHECK-DAG: expected string not found in input
// CHECK-DAG: ??2TypedefNewDelete@@SAPAXI at Z
```
SemaCXX/calling-conv-compat.cpp
```
error: 'error' diagnostics seen but not expected:
File /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-2/clang/test/SemaCXX/calling-conv-compat.cpp Line 367: cannot initialize a variable of type 'MemberPointers::fun_cdecl MemberPointers::A::*' with an rvalue of type 'void (MemberPointers::A::*)() __attribute__((thiscall))'
```
SemaCXX/decl-microsoft-call-conv.cpp
```
error: 'error' diagnostics expected but not seen:
File /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-2/clang/test/SemaCXX/decl-microsoft-call-conv.cpp Line 88: function declared 'cdecl' here was previously declared without calling convention
```
================
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:
> 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.)
Will do.
================
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;
----------------
rsmith wrote:
> 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.
Fixed. I forgot to add a check for ObjCGCAttrs when creating the type.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51329/new/
https://reviews.llvm.org/D51329
More information about the cfe-commits
mailing list