Page MenuHomePhabricator

[Attribute/Diagnostics] Print macro if definition is an attribute declaration
ClosedPublic

Authored by leonardchan on Aug 27 2018, 4:18 PM.

Details

Summary

If an address_space attribute is defined in a macro, print the macro instead when diagnosing a warning or error for incompatible pointers with different address_spaces.

Originally started from http://lists.llvm.org/pipermail/cfe-dev/2018-August/058702.html where the original proposal was a new pragma, but instead just changing the way address_space gets printed in a diagnostic.

We now allow this for all attributes (not just address_space), and for multiple attributes declared in the same macro.

Diff Detail

Repository
rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
leonardchan added inline comments.Aug 31 2018, 4:09 PM
lib/Lex/PPDirectives.cpp
2584 ↗(On Diff #163155)

See comment below

2720–2721 ↗(On Diff #163155)

Removed code relating to the processor since we don't need it anymore

lib/Parse/ParseDecl.cpp
114

Removed

117

The condition should instead be i < attrs.size(), sorry.

I don't know if ParsedAttributesView has a reverse iterator though or I can start with an iterator in the middle of the view.

229–235

Removed the preprocessor code

rsmith added a comment.Sep 4 2018, 4:51 PM

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.

lib/AST/ASTDiagnostic.cpp
82

Use castAs here and below (because it would be a programming error if the type isn't an AttributedType).

lib/AST/ASTImporter.cpp
991 ↗(On Diff #163604)

You need to import the identifier; the source and destination ASTs have different identifier tables. (Importer.Import(T->getMacroIdentifier())).

lib/AST/TypePrinter.cpp
1408

Incidentally, the last two arguments to the AddressSpaceAttr constructor in that patch (address space and spelling list index) are also reversed.

lib/Parse/ParseDecl.cpp
153

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();

227–235

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.

229–234

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 FileIDs 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.

mcgrathr added inline comments.Sep 4 2018, 5:16 PM
lib/Parse/ParseDecl.cpp
227–235

A single macro that uses multiple attributes is the central use case for us.
It would be fine if it's constrained to a single attribute clause (or [[...]] clause) with the attributes comma-separated, as in attribute((foo, bar)) as opposed to two separate attribute((foo)) attribute((bar)) in the macro, if that matters. It's even fine if it's constrained to the macro being nothing but the attribute((foo, bar)) clause (aside from whitespace and comments).

Leo can decide how he wants to proceed as far as incremental implementation.
But we won't have a real-world use for the feature only covering a single attribute.
We'll start using when it can cover the cases like the Linux user and iomem examples (address_space + noderef).

leonardchan marked 7 inline comments as done.Sep 10 2018, 9:31 AM

@rsmith So I chose to go with the type sugar route since we would prefer to have multiple attributes in one attribute list. This type just contains the underlying type and the macro identifier originally held by the AttributedType. Do you have any recommendations on where this type should be created?

Currently, I'm wrapping any AttributedType created under processTypeAttrs with this type since I still only hold the macro identifier in the ParsedAttr. The problem is that I keep incrementally running into failing assertions that I can slowly address, but all these assertions make me think I'm approaching this the wrong way. The failing assertions come from building or finding the type location if this type.

I'm thinking I could either make this type a subclass of AttributedType since we will essentially be using this only for attributes declared in a macro (I think this would allow me to not have to change some of the stuff that happens under GetTypeSourceInfoForDeclarator), or somehow find a way to make this in another part of Sema, although I would still need access to the parsed attributes so I would know what the macro identifiers are.

rsmith added a comment.Oct 3 2018, 5:56 PM

@rsmith So I chose to go with the type sugar route since we would prefer to have multiple attributes in one attribute list. This type just contains the underlying type and the macro identifier originally held by the AttributedType. Do you have any recommendations on where this type should be created?

I think the most natural thing to do would be to create the sugar node in processTypeAttrs after parsing each "chunk" of attributes produced by the same macro. (You'll need to extend ParsedAttributesView so it can tell you which ranges came from which macros.)

I'm thinking I could either make this type a subclass of AttributedType since we will essentially be using this only for attributes declared in a macro (I think this would allow me to not have to change some of the stuff that happens under GetTypeSourceInfoForDeclarator), or somehow find a way to make this in another part of Sema, although I would still need access to the parsed attributes so I would know what the macro identifiers are.

Your new type sugar node should be a new subclass of Type unrelated to AttributedType, maybe MacroQualifiedType. You'll need to teach TypeSpecLocFiller to fill in location information from it, but you can save that into the ParsedAttributesView when you parse the attributes and identify the macro expansions therein.

leonardchan marked an inline comment as done.
  • Added a new type sugar node MacroDefinedType which wraps AttributedTypes for the purpose of determining if an attribute was defined in a macro (this took much longer than I would've expected it to).
leonardchan added inline comments.Nov 7 2018, 4:50 PM
lib/Sema/SemaType.cpp
5952–5957

Moved the saved identifier into the new type sugar.

leonardchan added inline comments.Nov 7 2018, 4:51 PM
clang/lib/AST/TypePrinter.cpp
958–964 ↗(On Diff #173090)

This has not been addressed yet in this patch, but will still make another patch moving the address_space from the qualifier to the AttributedType (in reference to r340765).

leonardchan marked 2 inline comments as done.Jan 23 2019, 8:45 PM
leonardchan added inline comments.
clang/lib/AST/TypePrinter.cpp
958–964 ↗(On Diff #173090)

@rsmith Finally fixed in D55447, so I don't think there are any more outstanding prior comments that haven't been addressed in this patch so far.

rsmith added inline comments.Jan 28 2019, 5:49 PM
clang/include/clang/AST/Type.h
4183 ↗(On Diff #183242)

This represents a macro qualifier, not a type defined in a macro, so I don't think this name is appropriate. Maybe MacroQualifiedType?

4199 ↗(On Diff #183242)

Please also provide an accessor to get the unqualified type here (that is, the type that we would have had if the macro had not been written -- the original type of the AttributedType). Right now, that's (effectively) being computed in the type printer, which doesn't seem like the right place for that logic.

clang/lib/AST/Type.cpp
382–386 ↗(On Diff #183242)

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.

clang/lib/AST/TypePrinter.cpp
969–970 ↗(On Diff #183242)

This should apply to all attributes, not only address space attributes.

975–979 ↗(On Diff #183242)

This is the code that I mentioned above that should be moved to MacroDefinedType.

clang/lib/Parse/ParseDecl.cpp
252 ↗(On Diff #183242)

Typo "attatch"

257–262 ↗(On Diff #183242)

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.

clang/lib/Sema/SemaExpr.cpp
13667 ↗(On Diff #183242)

Please change TypeLoc::getAsAdjusted to skip macro type sugar instead of skipping it here. (getAsAdjusted already skips AttributedType sugar, and your new sugar node is the same kind of thing.)

Please also change Type::getAsAdjusted to likewise remove the new type node.

clang/lib/Sema/SemaStmt.cpp
3389–3395 ↗(On Diff #183242)

This duplicates the logic in TypeLoc::getAsAdjusted (and misses a case!). This whole function definition should be replaced by return FD->getTypeSourceInfo()->getTypeLoc().getAsAdjusted<FunctionProtoTypeLoc>().getReturnLoc();.

clang/lib/Sema/SemaType.cpp
6967–6972 ↗(On Diff #183242)

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.)

7553–7560 ↗(On Diff #183242)

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?

clang/test/Frontend/macro_defined_type.cpp
9 ↗(On Diff #183242)

Typo 'wether'

leonardchan marked 15 inline comments as done.
leonardchan added inline comments.
clang/lib/AST/Type.cpp
382–386 ↗(On Diff #183242)

Removed this method.

clang/lib/Parse/ParseDecl.cpp
257–262 ↗(On Diff #183242)

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.

clang/lib/Sema/SemaType.cpp
6967–6972 ↗(On Diff #183242)

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
7553–7560 ↗(On Diff #183242)

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 MacroQualifiedTypes for attributes that share the same name. Updated in this revision so that we continue to wrap AttributedTypes, 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.

leonardchan retitled this revision from [Attribute/Diagnostics] Print macro instead of whole attribute for address_space to [Attribute/Diagnostics] Print macro if definition is an attribute declaration.Feb 21 2019, 2:05 PM
leonardchan edited the summary of this revision. (Show Details)
rsmith added inline comments.Apr 29 2019, 4:06 PM
clang/include/clang/AST/Type.h
4174 ↗(On Diff #184439)

This comment is out of date. "[...] a type that was qualified by a qualifier written as a macro invocation" maybe?

4183 ↗(On Diff #184439)

Types should not carry source locations. If you want location information, it should be stored on the TypeLoc instead.

4199–4201 ↗(On Diff #184439)

getUnqualifiedType means something very specific in Clang, and this isn't it. getModifiedType would be a better name.

clang/include/clang/AST/TypeLoc.h
1086 ↗(On Diff #184439)

This struct is where the ExpansionLoc should live.

clang/lib/AST/TypePrinter.cpp
107–108 ↗(On Diff #184439)

I think this is more expensive than we need and unnecessary... see below.

982 ↗(On Diff #184439)

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.

clang/lib/Parse/ParseDecl.cpp
257–262 ↗(On Diff #183242)

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.)

clang/lib/Sema/SemaType.cpp
7373 ↗(On Diff #184439)

(Unused, please remove.)

6967–6972 ↗(On Diff #183242)

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.

7553–7560 ↗(On Diff #183242)

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.)

clang/lib/Sema/TreeTransform.h
6184 ↗(On Diff #184439)

(Copy the expansion loc here.)

clang/lib/Serialization/ASTReader.cpp
6181 ↗(On Diff #184439)

The name of this enumerator is out of date; should be TYPE_MACRO_QUALIFIED

clang/test/SemaObjC/gc-attributes.m
12–21 ↗(On Diff #184439)

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.

martong resigned from this revision.Apr 30 2019, 2:37 AM
leonardchan marked 17 inline comments as done.
leonardchan removed a reviewer: martong.
leonardchan added inline comments.Apr 30 2019, 12:39 PM
clang/lib/Parse/ParseDecl.cpp
257–262 ↗(On Diff #183242)

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.

clang/lib/Sema/SemaType.cpp
6967–6972 ↗(On Diff #183242)

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
7553–7560 ↗(On Diff #183242)

Will do.

clang/test/SemaObjC/gc-attributes.m
12–21 ↗(On Diff #184439)

Fixed. I forgot to add a check for ObjCGCAttrs when creating the type.

martong resigned from this revision.Apr 30 2019, 12:45 PM
rsmith added inline comments.Apr 30 2019, 2:17 PM
clang/lib/Parse/ParseDecl.cpp
257–262 ↗(On Diff #183242)

I'd like to get some (correct) form of this landed, so I wonder if we can do something much simpler for now. Can we just check that the first token of the attribute-specifier isAtStartOfMacroExpansion and the last token isAtEndOfMacroExpansion and that they're from the same expansion of the same (object-like) macro? That won't find the outermost such macro, nor will it deal with cases where __attribute__ or the last ) was itself generated by a macro, but it should not have any false positives.

leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
clang/lib/Parse/ParseDecl.cpp
257–262 ↗(On Diff #183242)

No problem. Done, and our original test cases still pass.

rsmith accepted this revision.May 2 2019, 12:56 PM

Thanks, looks great!

I think the handling of the expansion location in the MacroQualifiedTypeLoc isn't quite right yet (it looks like it will never actually be set at all, because the code to set it is not reachable) but it's also unused at the moment, so I'm happy with that being fixed as a follow-up change after you land this.

clang/lib/Sema/SemaType.cpp
5647 ↗(On Diff #197454)

This should use getMacroExpansionLoc on the relevant attribute (you can store the location information in TypeProcessingState and retrieve it from here; see what we do for AttributedType for example).

I think we'll also need a VisitMacroQualifiedTypeLoc override on TypeSpecLocFiller for attributes in the decl-specifiers rather than attributes on declarator chunks. (See below.)

5727–5728 ↗(On Diff #197454)

Skipping MacroQualifiedTypeLocs here means you're never reaching the above DeclaratorLocFiller::VisitMacroQualifiedTypeLoc.

This revision is now accepted and ready to land.May 2 2019, 12:56 PM

Thanks, looks great!

I think the handling of the expansion location in the MacroQualifiedTypeLoc isn't quite right yet (it looks like it will never actually be set at all, because the code to set it is not reachable) but it's also unused at the moment, so I'm happy with that being fixed as a follow-up change after you land this.

Thanks! Will make another patch eventually with the followup changes.

This revision was automatically updated to reflect the committed changes.

I've noticed it. Still looking at it. I'll revert this if I don't figure out what's causing it by the end of the night.

miyuki added a subscriber: miyuki.EditedMay 10 2019, 9:46 AM

This caused https://bugs.llvm.org/show_bug.cgi?id=41835
Could you please look into it?

This caused https://bugs.llvm.org/show_bug.cgi?id=41835
Could you please look into it?

Sorry for the delay. I submitted r360448 as a fix for this.

This caused a regression when including mingw-w64 headers (in a case where they happen to include redundant/duplicate attribute specifications), triggering asserts. See https://bugs.llvm.org/show_bug.cgi?id=41852 for details.

This caused a regression when including mingw-w64 headers (in a case where they happen to include redundant/duplicate attribute specifications), triggering asserts. See https://bugs.llvm.org/show_bug.cgi?id=41852 for details.

Thanks for the alert. I submitted r360544 which should address this.