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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
