[Attr] Fix parameter indexing for attributes

[jdenny]

Summary

The patch fixes a number of bugs related to parameter indexing in
attributes:

• Parameter indices in some attributes (argument_with_type_tag, pointer_with_type_tag, nonnull, ownership_takes, ownership_holds, and ownership_returns) are specified in source as one-origin including any C++ implicit this parameter, were stored as zero-origin excluding any this parameter, and were erroneously printing (-ast-print) and confusingly dumping (-ast-dump) as the stored values.

• For alloc_size, the C++ implicit this parameter was not subtracted correctly in Sema, leading to assert failures or to silent failures of __builtin_object_size to compute a value.

• For argument_with_type_tag, pointer_with_type_tag, and ownership_returns, the C++ implicit this parameter was not added back to parameter indices in some diagnostics.

This patch fixes the above bugs and aims to prevent similar bugs in
the future by introducing careful mechanisms for handling parameter
indices in attributes. ParamIdx stores a parameter index and is
designed to hide the stored encoding while providing accessors that
require each use (such as printing) to make explicit the encoding that
is needed. Attribute declarations declare parameter index arguments
as [Variadic]ParamIdxArgument, which are exposed as ParamIdx[*]. This
patch rewrites all attribute arguments that are processed by
checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp to be declared
as [Variadic]ParamIdxArgument. The only exception is xray_log_args's
argument, which is encoded as a count not an index.

An alternative approach is to attempt to hide the issue of parameter
index encoding at each use by somehow implicitly exposing exactly the
encoding needed. That approach is essentially the approach that
already existed, and I made several attempts to improve it, but I have
concluded that it is both futile and harmful. The trouble is that the
needed encoding varies among attribute arguments and even among uses
of a single attribute argument. Making the exposed encoding implicit
then requires developers to think about not only what encoding is
needed at every use but also what encoding is implicitly being
exposed. Thus, making the encoding explicit at every use should
actually reduce the cognitive burden and the potential for mistakes.

[aaron.ballman]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

Is there much benefit to forcing the attribute author to pick a name for this? It seems like this is more of a Boolean value: either implicit this is allowed or not. It would be really nice if we could hide these mechanics as implementation details so that the user only needs to write VariadicParamIndexArgument<"name", DisallowImplicitThis> (or something similar) when defining the attribute, and ideally not have to do any extra work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if possible.

lib/Sema/SemaDeclAttr.cpp

355–357 ↗

(On Diff #134085)

Formatting is off here -- the else if should go up a line.

4612 ↗

(On Diff #134085)

This value cannot be different from the previously-read value, correct? Might want to assert that.

test/Sema/attr-print.cpp

1 ↗

(On Diff #134085)

-ast-print tests generally live in Misc rather than Sema -- can you move this test over to that directory?

33 ↗

(On Diff #134085)

Can you add some tests for __attribute__((format)) and __attribute__((format_arg)) to demonstrate that their pretty-printed argument values are also sensible?

[jdenny]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

Thanks for the review. I'll work on that.

lib/Sema/SemaDeclAttr.cpp

355–357 ↗

(On Diff #134085)

I don't follow. It looks right to me.

4612 ↗

(On Diff #134085)

Right. Hopefully this will go away after I apply your initial suggestion.

test/Sema/attr-print.cpp

1 ↗

(On Diff #134085)

Sure. Should the existing test/Sema/attr-print.c move too?

33 ↗

(On Diff #134085)

Will look into it.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

355–357 ↗

(On Diff #134085)

Our coding standard is to write:

if (foo) {
} else if (bar) {
}

rather than

if (foo) {
}
else if (bar) {
}

One good way to catch this sort of thing is to run your patch through clang-format: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

test/Sema/attr-print.cpp

1 ↗

(On Diff #134085)

Ah, I didn't see we already had that one here. In that case, it's fine to leave this where it is.

[jdenny]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

Is there much benefit to forcing the attribute author to pick a name for this? It seems like this is more of a Boolean value: either implicit this is allowed or not.

If the attribute author picks the name, then the attribute author can ensure there's only one of these per attribute. I could rewrite it to have one per VariadicParamIndexArgument and one per ParamIndexArgument if you like. In that case, ArgumentWithTypeTagAttr would end up with two of these, and future attributes could potentially have more, but they should all have the same value within a single attribute. I didn't investigate how that redundancy would actually impact memory usage. What do you think?

It would be really nice if we could hide these mechanics as implementation details so that the user only needs to write VariadicParamIndexArgument<"name", DisallowImplicitThis> (or something similar) when defining the attribute, and ideally not have to do any extra work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if possible.

So far, I haven't found a good way to accomplish that, or maybe I've misunderstood you....

The logic of checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp seems pretty tightly coupled with its users' logic. For example, handleNonNullAttr uses the indices as adjusted by checkFunctionOrMethodParameterIndex to determine which indices belong in the array to be passed to the NonNullAttr constructor. We could try to have NonNullAttr (in the constructor, presumably) perform the adjustment of indices so that SemaDeclAttr.cpp doesn't need that logic, but then it would be too late to for handleNonNullAttr to filter them.

The only extra work this patch adds to SemaDeclAttr.cpp beyond what's already there is to reuse the DisallowImplicitThis that is essentially already computed in checkFunctionOrMethodParameterIndex.

Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index adjustment logic and pass an index offset to attribute constructors, so ClangAttrEmitter.cpp would just know it has to print indices with some given offset. But a general index offset is wider than the single bool being stored now. Again, I haven't investigated the actual impact on memory usage.

Do you see a better way to achieve the encapsulation you're looking for?

lib/Sema/SemaDeclAttr.cpp

355–357 ↗

(On Diff #134085)

Thanks.

[aaron.ballman]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

In that case, ArgumentWithTypeTagAttr would end up with two of these, and future attributes could potentially have more, but they should all have the same value within a single attribute. I didn't investigate how that redundancy would actually impact memory usage. What do you think?

That redundancy could probably be worked around in ClangAttrEmitter.cpp by inspecting the attribute argument list and noticing duplicate [Variadic]ParamIndexArgument before generating the code for the arguments, perhaps?

Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index adjustment logic and pass an index offset to attribute constructors, so ClangAttrEmitter.cpp would just know it has to print indices with some given offset.

This was more along the lines of what I was thinking of.

Basically, it seems like the declarative part in Attr.td should be able to specify an attribute argument is intended to be a function parameter positional index, and whether implicit this needs special handling or not. Past that, the semantics of the attribute really shouldn't matter -- when asking for the index, it should be automatically adjusted so that users of the attribute don't have to do anything, and when printing the attribute, the index should be appropriately adjusted back. I'd like to avoid needing collusion between the declarative part in Attr.td and the semantic part in SemaDeclAttr.cpp because that collusion can get out of sync. It feels like we should be able to hide that collusion in ClangAttrEmitter.cpp by using some extra flags on the attribute that get set (automatically) during construction. The way you have things set up right now is very close to this, except the information has to be passed to the constructor manually in SemaDeclAttr after being calculated by checkFunctionOrMethodParameterIndex().

I'm not too worried about the memory usage at this point; if it looks problematic, we can likely address it.

[jdenny]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

That redundancy could probably be worked around in ClangAttrEmitter.cpp by inspecting the attribute argument list and noticing duplicate [Variadic]ParamIndexArgument before generating the code for the arguments, perhaps?

Seems reasonable.

There's also the question of the constructor interface: does it accept the same information multiple times or one time?

For now, the memory optimization doesn't seem too important, so I won't work on detecting duplicates. If we implement that later, we should be able to easily adjust callers for the resulting interface change.

Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index adjustment logic and pass an index offset to attribute constructors, so ClangAttrEmitter.cpp would just know it has to print indices with some given offset.

This was more along the lines of what I was thinking of.

Sorry, does "This" refer to the alternative proposal I just described above or your following paragraph?

Basically, it seems like the declarative part in Attr.td should be able to specify an attribute argument is intended to be a function parameter positional index, and whether implicit this needs special handling or not.

Attr.td cannot specify declaratively whether implicit this needs special handling (I've been calling this DisallowImplicitThisParam, but let's call it AdjustForImplicitThis for clarity) because part of that decision is per function: whether the function actually has an implicit this (HasImplicitThis). Attr.td might always be able to specify whether implicit this is allowed (AllowImplicitThis) and thus, when there actually is an implicit this (HasImplicitThis), whether an extra increment should be added for printing (AdjustForImplicitThis = HasImplicitThis && !AllowImplicitThis). ClangAttrEmitter.cpp would then have to learn how to compute AdjustForImplicitThis from the other two Booleans, but SemaDeclAttr.cpp already does that.

In my current patch, only SemaDeclAttr.cpp understands how to compute AdjustForImplicitThis, but ClangAttrEmitter.cpp documents it too and knows there's always another increment (to make indices one-origin) for printing. In my alternative proposal above, ClangAttrEmitter.cpp would not document or understand any of this. Instead, ClangAttrEmitter.cpp would only know that indices must be adjusted by some arbitrary value for the sake of printing, and SemaDeclAttr.cpp would tell it how much. That seems like the cleanest separation.

Past that, the semantics of the attribute really shouldn't matter -- when asking for the index, it should be automatically adjusted so that users of the attribute don't have to do anything, and when printing the attribute, the index should be appropriately adjusted back.

Agreed. Both my current patch and my alternative proposal above offer that.

I'd like to avoid needing collusion between the declarative part in Attr.td and the semantic part in SemaDeclAttr.cpp because that collusion can get out of sync.

The only collusion I see in my current patch (besides some perhaps unnecessary documentation in ClangAttrEmitter.cpp) is the understanding that the Boolean indicates that one increment beyond the usual single increment is needed when printing. I believe my alternative proposal eliminates even that: then there's just some arbitrary increment when printing, and ClangAttrEmitter.cpp doesn't know or care why.

It feels like we should be able to hide that collusion in ClangAttrEmitter.cpp by using some extra flags on the attribute that get set (automatically) during construction. The way you have things set up right now is very close to this, except the information has to be passed to the constructor manually in SemaDeclAttr after being calculated by checkFunctionOrMethodParameterIndex().

In my last response, I was explaining why checkFunctionOrMethodParameterIndex probably needs to calculate that information anyway for the sake of its users, and it seems that having both SemaDeclAttr and ClangAttrEmitter know how to calculate it is worse for maintenance than having SemaDeclAttr pass its result to ClangAttrEmitter. My alternative proposal above does not require ClangAttrEmitter to calculate the adjustment at all. It just receives the net adjustment from SemaDeclAttr.

I feel like my alternative proposal would satisfy your desire for a better separation of concerns. The difference from what I think you're describing is that my alternative proposal keeps the index adjustment amount computation in SemaDeclAttr rather than trying to move any of that computation to ClangAttrEmitter. Does that seem reasonable to you?

[aaron.ballman]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

Sorry, does "This" refer to the alternative proposal I just described above or your following paragraph?

Your proposal, which I thought was solving what I described in the following paragraph.

Attr.td might always be able to specify whether implicit this is allowed (AllowImplicitThis) and thus, when there actually is an implicit this (HasImplicitThis), whether an extra increment should be added for printing (AdjustForImplicitThis = HasImplicitThis && !AllowImplicitThis). ClangAttrEmitter.cpp would then have to learn how to compute AdjustForImplicitThis from the other two Booleans, but SemaDeclAttr.cpp already does that.

Okay, I think I see where we may not quite be on the same page. The Attr.td bit knows whether the attribute needs to adjust for implicit this or not, but only SemaDeclAttr.cpp knows about the actual function the attribute is trying to be applied to. What I was envisioning was to move the logic from SemaDeclAttr.cpp into something accessible from the code generated by ClangAttrEmitter.cpp, but that may not be sufficient because the semantic attribute needs that information from the specific Decl, which by definition requires something additional to be passed in from SemaDeclAttr.cpp.

Thanks for bearing with me. :-)

I feel like my alternative proposal would satisfy your desire for a better separation of concerns. The difference from what I think you're describing is that my alternative proposal keeps the index adjustment amount computation in SemaDeclAttr rather than trying to move any of that computation to ClangAttrEmitter. Does that seem reasonable to you?

I think it's reasonable, depending on how it's surfaced. Would it be reasonable to make it look something like this?

// Attr.td
// ...
  let Args = [VariadicParamIndexArgument<"Args", AdjustsImplicitThis>];
// ...

// SemaDeclAttr.cpp
// ...
ImplicitThisAdjustment AdjustsImplicitThis;
if (!checkFunctionOrMethodParameterIndex(..., ..., AdjustsImplicitThis))
  return;

// ...
D->addAttr(::new (S.Context) NonNullAttr(..., ..., AdjustsImplicitThis));

(I'm using enumerations in both files rather than Boolean values so it's more clear what's happening).

This implies that any attribute accepting a parameter index will have a constructor that requires you to specify whether that instance of the attribute adjusts implicit this or not.

[jdenny]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

What I was envisioning was to move the logic from SemaDeclAttr.cpp into something accessible from the code generated by ClangAttrEmitter.cpp, but that may not be sufficient because the semantic attribute needs that information from the specific Decl, which by definition requires something additional to be passed in from SemaDeclAttr.cpp.

Well, maybe we can do that. ClangAttrEmitter.cpp could generate for the attribute a static member function that massages one index into the form users expect. That function's parameters would be the original index and HasImplicitThis (or maybe the function Decl itself). AllowsImplicitThis would be declared in Attr.td as you want. checkFunctionOrMethodParameterIndex could use that function to compute each index before it examines it, and callers could pass that resulting index to the attribute constructor. It seems slightly messy because users of the attribute must know to call that function on indices before passing them to the constructor, but it means ClangAttrEmitter.cpp contains all the index adjustment logic, and Attr.td declares the property AllowsImplicitThis, which I agree feels like a static property of an attribute.

At this point, I think I'll just write a patch and see what you think. We can adjust if it's still too ugly.

Thanks for bearing with me. :-)

Likewise. It sounds like we're converging.

[aaron.ballman]

include/clang/Basic/Attr.td

182 ↗

(On Diff #134085)

At this point, I think I'll just write a patch and see what you think. We can adjust if it's still too ugly.

I think that sounds like a good idea.

[jdenny]

After several attempts at strategies Aaron and I discussed, I ended up going a different way. See the new summary for details. I believe this version addresses all the issues Aaron raised so far.

[aaron.ballman]

(Review is incomplete, but here are some initial comments.)

include/clang/AST/Attr.h

210–212 ↗

(On Diff #135839)

I think it might be best to mash these together using bit-fields:

unsigned Idx : 30;
unsigned HasThis : 1;
unsigned IsValid : 1;

218 ↗

(On Diff #135839)

Add some newlines between the various declarations in the class -- everything is condensed a bit too much without it.

225 ↗

(On Diff #135839)

const auto *

227–228 ↗

(On Diff #135839)

Might as well move this into the member initializer.

238–243 ↗

(On Diff #135839)

Is this necessary? There should be an implicit copy assignment operator for this class, and it's a bit strange to have a copy assignment but no copy constructor.

[aaron.ballman]

include/clang/AST/Attr.h

264 ↗

(On Diff #135839)

I'd prefer more clear names, like "getSourceIndex()" and "getASTIndex()".

282 ↗

(On Diff #135839)

Perhaps getLLVMIndex() for this one.

291–292 ↗

(On Diff #135839)

std::iterator was deprecated in C++17, so you should manually expose these fields.

[jdenny]

This revision should address all issues raised.

include/clang/AST/Attr.h

210–212 ↗

(On Diff #135839)

Good point. Thanks.

238–243 ↗

(On Diff #135839)

Oops. Thanks.

291–292 ↗

(On Diff #135839)

Now that sizeof(ParamIdx) == sizeof(unsigned), I can no longer find a reason to avoid ParamIdx arrays, and so I can no longer find a good justification for the iterator classes.

[jdenny]

Hi Aaron. It occurs to me now that this patch has grown rather large and, in some places, a little subtle. Would it help the review if I were to break it up into a patch series that introduces ParamIdx to each attribute, one at a time? I'm not trying to rush you, but I hate for the review to be painful for you if it doesn't have to be.

[aaron.ballman]

include/clang/AST/Attr.h

206 ↗

(On Diff #136019)

The name here can be improved. How about checkInvariants()? Might as well make this inline while you're at it.

236 ↗

(On Diff #136019)

Is this constructor used anywhere? I didn't see it being used, but I could have missed something. If it's not used, go ahead and remove it.

267 ↗

(On Diff #136019)

Please assert that Idx won't wrap before doing the return.

276 ↗

(On Diff #136019)

Likewise here.

281–284 ↗

(On Diff #136019)

Are all of these operations required for the class?

include/clang/Basic/Attr.td

172–174 ↗

(On Diff #136019)

I still find the AllowThis parts to be hard to follow, so I want to be sure I understand your design properly. Everything that uses this new argument type sets AllowsThis to 0. As written, it sounds like setting that to 0 means that the parameter index cannot be used on a C++ instance method, and I know that's not the correct interpretation. Under what circumstances would this be set to 1 instead?

Looking at the existing code, the only circumstances under which checkFunctionOrMethodParameterIndex() was being passed true for "allow this" was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this member isn't necessary any longer?

lib/Sema/SemaChecking.cpp

2622 ↗

(On Diff #136019)

const ParamIdx &

10083 ↗

(On Diff #136019)

const ParamIdx &

12244 ↗

(On Diff #136019)

Hoist the AST index so you don't have to call for it twice. (Same applies elsewhere.)

lib/Sema/SemaDeclAttr.cpp

785–786 ↗

(On Diff #136019)

Good catch about this not being needed any longer!

1650–1651 ↗

(On Diff #136019)

This change looks to be unrelated to the patch?

4549–4551 ↗

(On Diff #136019)

Is this formatting produced by clang-format?

lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

61 ↗

(On Diff #136019)

const ParamIdx &

[aaron.ballman]

In D43248#1023720, @jdenny wrote:

Hi Aaron. It occurs to me now that this patch has grown rather large and, in some places, a little subtle. Would it help the review if I were to break it up into a patch series that introduces ParamIdx to each attribute, one at a time? I'm not trying to rush you, but I hate for the review to be painful for you if it doesn't have to be.

No need to do that -- this review just takes a bit more time for me to complete, but it's reasonably well-factored. Thank you, though!

[jdenny]

In D43248#1024183, @aaron.ballman wrote:

In D43248#1023720, @jdenny wrote:

Hi Aaron. It occurs to me now that this patch has grown rather large and, in some places, a little subtle. Would it help the review if I were to break it up into a patch series that introduces ParamIdx to each attribute, one at a time? I'm not trying to rush you, but I hate for the review to be painful for you if it doesn't have to be.

No need to do that -- this review just takes a bit more time for me to complete, but it's reasonably well-factored. Thank you, though!

Sure! Thanks for the review.

include/clang/AST/Attr.h

206 ↗

(On Diff #136019)

Sure, I can change the name.

It's inside the class, so specifying inline is against the LLVM coding standards, right?

236 ↗

(On Diff #136019)

It's used by the deserialization code generated by ClangAttrEmitter.cpp.

281–284 ↗

(On Diff #136019)

operator== and operator< are needed for sorting and finding. It seems strange to me not to finish the set.

include/clang/Basic/Attr.td

172–174 ↗

(On Diff #136019)

I still find the AllowThis parts to be hard to follow, so I want to be sure I understand your design properly. Everything that uses this new argument type sets AllowsThis to 0. As written, it sounds like setting that to 0 means that the parameter index cannot be used on a C++ instance method, and I know that's not the correct interpretation.

Right. AllowsThis=0 means it is an error for the attribute in the source code to specify the C++ implicit this parameter (index 1).

Under what circumstances would this be set to 1 instead?

Looking at the existing code, the only circumstances under which checkFunctionOrMethodParameterIndex() was being passed true for "allow this" was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this member isn't necessary any longer?

Right. I also noticed this issue, but I probably should have mentioned that in a comment in the source instead of just rewording the last paragraph of the patch summary. Sorry.

I thought about removing AllowsThis, but I hesitated because I had already implemented it by the time I noticed this issue and because I assumed there must be some reason why attributes for C++ have index 1 mean the this parameter, so there might be some attribute that could eventually take advantage of AllowsThis=1. Moreover, it makes the related argument to checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer.

I don't feel strongly either way, so I'm happy to remove it or keep it.

lib/Sema/SemaDeclAttr.cpp

1650–1651 ↗

(On Diff #136019)

Sorry, I think clang-format suggested that at some point in history, and I overlooked it when reviewing the latest version.

4549–4551 ↗

(On Diff #136019)

Yes.

[aaron.ballman]

include/clang/AST/Attr.h

206 ↗

(On Diff #136019)

Derp, you're correct, it's already implicitly inline. Ignore that part of the suggestion.

236 ↗

(On Diff #136019)

That'd explain why I hadn't seen it.

281–284 ↗

(On Diff #136019)

I don't think it's actively harmful to do so, but I also don't think it's really needed either. Your call.

include/clang/Basic/Attr.td

172–174 ↗

(On Diff #136019)

Right. AllowsThis=0 means it is an error for the attribute in the source code to specify the C++ implicit this parameter (index 1).

Then if we keep this functionality, I think a better identifier would be something like CanIndexImplicitThis and the comments could be updated to more clearly state what is allowed/disallowed. Then the other uses of "allow this" can be updated to use similar terminology for clarity.

so there might be some attribute that could eventually take advantage of AllowsThis=1. Moreover, it makes the related argument to checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer.

My gut feeling is that this functionality isn't going to be needed all that frequently. If we get a second case where we need it, then I'd say it might make more sense to add it.

Attributes that use positional arguments should hopefully be the exception, not the rule, because those indexes are horribly fragile.

What do you think?

lib/Sema/SemaDeclAttr.cpp

4549–4551 ↗

(On Diff #136019)

How funky. :-)

[jdenny]

include/clang/Basic/Attr.td

172–174 ↗

(On Diff #136019)

My gut feeling is that this functionality isn't going to be needed all that frequently. If we get a second case where we need it, then I'd say it might make more sense to add it.

Attributes that use positional arguments should hopefully be the exception, not the rule, because those indexes are horribly fragile.

What do you think?

I'm guessing you have more experience with attributes in general, so let's go with your gut. :-)

lib/Sema/SemaDeclAttr.cpp

4549–4551 ↗

(On Diff #136019)

Agreed.

[jdenny]

This update should address all outstanding comments.

include/clang/AST/Attr.h

206 ↗

(On Diff #136019)

The name here can be improved. How about checkInvariants()?

I went with assertComparable because, in my view, this is not so much a class invariant as it is an assertion about correct usage of comparison operators. But I'm not married to it.

[aaron.ballman]

Aside from two minor nits, this LGTM. Thank you for working on it!

test/Sema/attr-ownership.cpp

1 ↗

(On Diff #136624)

Please pass -fsyntax-only as well.

utils/TableGen/ClangAttrEmitter.cpp

763 ↗

(On Diff #136624)

Given how oddly this is wrapped, might as well make this a stream argument rather than a string literal concatenation.

[jdenny]

test/Sema/attr-ownership.cpp

1 ↗

(On Diff #136624)

Sure. Would you like me to change test/Sema/attr-ownership.c while we're thinking about it?

[aaron.ballman]

test/Sema/attr-ownership.cpp

1 ↗

(On Diff #136624)

Because it's unrelated to this patch, that can be done in a separate commit (no review required, you can just make the change and commit it).

[jdenny]

Aaron, thanks for the review. I've applied your suggestions and am ready to commit.

I've noticed a variety of styles in commit logs, I've read the coding standards, and it seems there's a lot of freedom here. Below are the two commit logs I'm planning to use. Please let me know if you have any objections. Thanks.

[Attr] Fix parameter indexing for several attributes

The patch fixes a number of bugs related to parameter indexing in
attributes:

* Parameter indices in some attributes (argument_with_type_tag,
  pointer_with_type_tag, nonnull, ownership_takes, ownership_holds,
  and ownership_returns) are specified in source as one-origin
  including any C++ implicit this parameter, were stored as
  zero-origin excluding any this parameter, and were erroneously
  printing (-ast-print) and confusingly dumping (-ast-dump) as the
  stored values.

* For alloc_size, the C++ implicit this parameter was not subtracted
  correctly in Sema, leading to assert failures or to silent failures
  of __builtin_object_size to compute a value.

* For argument_with_type_tag, pointer_with_type_tag, and
  ownership_returns, the C++ implicit this parameter was not added
  back to parameter indices in some diagnostics.

This patch fixes the above bugs and aims to prevent similar bugs in
the future by introducing careful mechanisms for handling parameter
indices in attributes.  ParamIdx stores a parameter index and is
designed to hide the stored encoding while providing accessors that
require each use (such as printing) to make explicit the encoding that
is needed.  Attribute declarations declare parameter index arguments
as [Variadic]ParamIdxArgument, which are exposed as ParamIdx[*].  This
patch rewrites all attribute arguments that are processed by
checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp to be declared
as [Variadic]ParamIdxArgument.  The only exception is xray_log_args's
argument, which is encoded as a count not an index.

Differential Revision: https://reviews.llvm.org/D43248

For the test/Sema/attr-ownership.c change:

[Attr] Use -fsyntax-only in test

Suggested at: https://reviews.llvm.org/D43248

[aaron.ballman]

In D43248#1025495, @jdenny wrote:

Aaron, thanks for the review. I've applied your suggestions and am ready to commit.

You're correct that we have a lot of freedom with the commit message, but the important piece is in describing what's changed and why (if it's not immediately obvious). Both commit messages look good to me.

[jdenny]

This commit was reverted by r326862 due to:

https://bugs.llvm.org/show_bug.cgi?id=36620

This revision includes a new test case and a fix.

While the difference from the last revision is small, it's not trivial, so another review is probably worthwhile. The main difference is two new ParamIdx member functions: serialize and deserialize. In ClangAttrEmitter.cpp, these functions enable significant simplifications and facilitate the bug fix.

[aaron.ballman]

Can you regenerate the patch using the same paths as https://reviews.llvm.org/D43248?id=136811? When I try to do a diff between what was accepted & committed and the current patch, Phabricator gets confused because the paths are too different from one another.

[jdenny]

OK, this diff has the svn paths, and I've rebased to a more recent master.

[jdenny]

Well, that didn't work. Here's another attempt at getting the paths right.

[aaron.ballman]

It seems like there are some other changes than just the serialize and deserialize that I'm not opposed to, but am wondering why they're needed. It seems some functions are now getFoo() calls and it seems like some declarations moved around. Are those intended as part of this patch?

cfe/trunk/test/Frontend/ast-attr.cpp

1–2 ↗

(On Diff #138113)

Can you move this below the RUN line?

cfe/trunk/test/Sema/attr-print.cpp

62

Can you move this comment below the RUN line?

[jdenny]

In D43248#1035466, @aaron.ballman wrote:

It seems like there are some other changes than just the serialize and deserialize that I'm not opposed to, but am wondering why they're needed. It seems some functions are now getFoo() calls

These were originally named getFoo, and my previous patch changed them to foo. I believe I did that to make ParamIdxArgument accessors more like VariadicParamIdxArgument accessors (which inherits accessors from VariadicArgument), but I probably shouldn't have done that. In any case, this new revision implements ParamIdxArgument using SimpleArgument, and that names accessors like getFoo.

and it seems like some declarations moved around. Are those intended as part of this patch?

Are you referring to the changes in SemaDeclAttr.cpp? Those changes are needed because the ParamIdx constructor now asserts that Idx is one-origin, but that requires validating that it's actually one-origin beforehand. Sorry, I should've mentioned the new asserts.

[jdenny]

cfe/trunk/test/Frontend/ast-attr.cpp

1–2 ↗

(On Diff #138113)

Sure. I'm still trying to learn the LLVM coding standards. Is this specified there?

[aaron.ballman]

In D43248#1035477, @jdenny wrote:

In D43248#1035466, @aaron.ballman wrote:

It seems like there are some other changes than just the serialize and deserialize that I'm not opposed to, but am wondering why they're needed. It seems some functions are now getFoo() calls

These were originally named getFoo, and my previous patch changed them to foo. I believe I did that to make ParamIdxArgument accessors more like VariadicParamIdxArgument accessors (which inherits accessors from VariadicArgument), but I probably shouldn't have done that. In any case, this new revision implements ParamIdxArgument using SimpleArgument, and that names accessors like getFoo.

Ahhh, thank you for the explanation, that makes sense.

and it seems like some declarations moved around. Are those intended as part of this patch?

Are you referring to the changes in SemaDeclAttr.cpp? Those changes are needed because the ParamIdx constructor now asserts that Idx is one-origin, but that requires validating that it's actually one-origin beforehand. Sorry, I should've mentioned the new asserts.

Ah, okay, thank you!

cfe/trunk/test/Frontend/ast-attr.cpp

1–2 ↗

(On Diff #138113)

Nope! I'm just used to looking at the very first line of the test to know what it's running, and that seems consistent with other tests.

[jdenny]

cfe/trunk/test/Frontend/ast-attr.cpp

1–2 ↗

(On Diff #138113)

OK, that makes sense. I'll be sure to change both of those.

[aaron.ballman]

cfe/trunk/test/Frontend/ast-attr.cpp

5 ↗

(On Diff #138113)

Just to verify my understanding, this test is checking the serialization and deserialization?

[jdenny]

cfe/trunk/test/Frontend/ast-attr.cpp

5 ↗

(On Diff #138113)

That's right, and it failed without the other changes in this revision because alloc_size's second argument serialized/deserialized as 0 when it was actually invalid (that is, unspecified). Of course, this test is more thorough than just exercising alloc_size.

[aaron.ballman]

LGTM with the test comments fixed up.

cfe/trunk/test/Frontend/ast-attr.cpp

5 ↗

(On Diff #138113)

Perfect, thank you!

[jdenny]

In D43248#1035509, @aaron.ballman wrote:

LGTM with the test comments fixed up.

Thanks! I'll commit tomorrow.

[echristo]

test/Sema/attr-print.cpp

3 ↗

(On Diff #138193)

Relatedly I don't think we use files as input files to other directories and I don't think we should really. What are you trying to test here? This breaks the hermeticness of each particular test directory.

[jdenny]

test/Sema/attr-print.cpp

3 ↗

(On Diff #138193)

Grep for "%S/.." and you'll see that a few other tests do something like this as well.

In test/Sema/attr-print.cpp, I am testing printing of attributes. I chose to put that there because of the existing attr-print.c there.

In test/Frontend/ast-attr.cpp, I am testing serialization of attributes. I chose to put that there because I see other -emit-ast tests there and because, if I put it in test/Sema instead, I get the complaint "Do not use the driver in Sema tests".

The same C++ code makes sense in both of these, but replicating it in two files would worsen maintainability.

I could try to combine into one file in, say, tests/Misc if that works.

I have no strong opinions on where these live. Just for my own education, is something real breaking right now because of their current locations?

Thanks.

[echristo]

test/Sema/attr-print.cpp

3 ↗

(On Diff #138193)

Basically it's breaking an external build system (bazel) that has fairly distinct and specific dependency requirements and so layering and other dependencies are pretty entertaining.

Right now we avoid testing those particular tests and have TODOs of fixing them and the requirements to use a single directory and I was trying to avoid one more here.

All of that said I totally understand the desire to keep the maintenance burden minimized and an external build system shouldn't affect whether or not we do one particular thing or another - I was trying to get it written so that we could enable it without much undue burden. I'm uncertain if a comment of:

// If you change this file you should also change blah file.

or moving it to another directory where you can do both tests at the same time.

[jdenny]

I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says.

[aaron.ballman]

In D43248#1036409, @jdenny wrote:

I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says.

I have a slight preference for moving the tests now that I know they're causing problems, unless that turns out to be overly onerous for some reason.

Thank you, @echristo for pointing out the issues with the tests, I hadn't considered that.

[echristo]

In D43248#1036426, @aaron.ballman wrote:

In D43248#1036409, @jdenny wrote:

I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says.

I have a slight preference for moving the tests now that I know they're causing problems, unless that turns out to be overly onerous for some reason.

Thank you, @echristo for pointing out the issues with the tests, I hadn't considered that.

No worries :)

That said, please don't revert and reapply to move. Just a followup commit is just fine - and whenever you get a chance. (Though if you let me know when you do I'd appreciate it :)

[jdenny]

In D43248#1036427, @echristo wrote:

In D43248#1036426, @aaron.ballman wrote:

In D43248#1036409, @jdenny wrote:

I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says.

I have a slight preference for moving the tests now that I know they're causing problems, unless that turns out to be overly onerous for some reason.

Thank you, @echristo for pointing out the issues with the tests, I hadn't considered that.

No worries :)

That said, please don't revert and reapply to move. Just a followup commit is just fine - and whenever you get a chance. (Though if you let me know when you do I'd appreciate it :)

Sure.

So, I'm planning to remove test/Frontend/ast-attr.cpp, rename test/Sema/attr-print.cpp to test/Misc/attr-print-emit.cpp, and change its run lines to:

// RUN: %clang_cc1 %s -ast-print | FileCheck %s
// RUN: %clang -emit-ast -o %t.ast %s
// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %s

That seems to work fine locally. I'll just leave the old test/Sema/attr-print.c alone as it's not part of this problem. Any objections?

[jdenny]

In D43248#1036439, @jdenny wrote:

So, I'm planning to remove test/Frontend/ast-attr.cpp, rename test/Sema/attr-print.cpp to test/Misc/attr-print-emit.cpp, and change its run lines to:

// RUN: %clang_cc1 %s -ast-print | FileCheck %s
// RUN: %clang -emit-ast -o %t.ast %s
// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %s

That seems to work fine locally. I'll just leave the old test/Sema/attr-print.c alone as it's not part of this problem. Any objections?

I went ahead and committed. It's in r327456.

cfe/trunk/include/clang/AST/Attr.h

public:
   }
 
   static bool classof(const Attr *A) {
     return A->getKind() >= attr::FirstParameterABIAttr &&
            A->getKind() <= attr::LastParameterABIAttr;
    }
 };
 
+/// A single parameter index whose accessors require each use to make explicit
+/// the parameter index encoding needed.
+class ParamIdx {
+  // Idx is exposed only via accessors that specify specific encodings.
+  unsigned Idx : 30;
+  unsigned HasThis : 1;
+  unsigned IsValid : 1;
+
+  void assertComparable(const ParamIdx &I) const {
+    assert(isValid() && I.isValid() &&
+           "ParamIdx must be valid to be compared");
+    // It's possible to compare indices from separate functions, but so far
+    // it's not proven useful.  Moreover, it might be confusing because a
+    // comparison on the results of getASTIndex might be inconsistent with a
+    // comparison on the ParamIdx objects themselves.
+    assert(HasThis == I.HasThis &&
+           "ParamIdx must be for the same function to be compared");
+  }
+
+public:
+  /// Construct an invalid parameter index (\c isValid returns false and
+  /// accessors fail an assert).
+  ParamIdx() : Idx(0), HasThis(false), IsValid(false) {}
+
+  /// \param Idx is the parameter index as it is normally specified in
+  /// attributes in the source: one-origin including any C++ implicit this
+  /// parameter.
+  ///
+  /// \param D is the declaration containing the parameters.  It is used to
+  /// determine if there is a C++ implicit this parameter.
+  ParamIdx(unsigned Idx, const Decl *D)
+      : Idx(Idx), HasThis(false), IsValid(true) {
+    if (const auto *FD = dyn_cast<FunctionDecl>(D))
+      HasThis = FD->isCXXInstanceMember();
+  }
+
+  /// \param Idx is the parameter index as it is normally specified in
+  /// attributes in the source: one-origin including any C++ implicit this
+  /// parameter.
+  ///
+  /// \param HasThis specifies whether the function has a C++ implicit this
+  /// parameter.
+  ParamIdx(unsigned Idx, bool HasThis)
+      : Idx(Idx), HasThis(HasThis), IsValid(true) {}
+
+  /// Is this parameter index valid?
+  bool isValid() const { return IsValid; }
+
+  /// Is there a C++ implicit this parameter?
+  bool hasThis() const {
+    assert(isValid() && "ParamIdx must be valid");
+    return HasThis;
+  }
+
+  /// Get the parameter index as it would normally be encoded for attributes at
+  /// the source level of representation: one-origin including any C++ implicit
+  /// this parameter.
+  ///
+  /// This encoding thus makes sense for diagnostics, pretty printing, and
+  /// constructing new attributes from a source-like specification.
+  unsigned getSourceIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    return Idx;
+  }
+
+  /// Get the parameter index as it would normally be encoded at the AST level
+  /// of representation: zero-origin not including any C++ implicit this
+  /// parameter.
+  ///
+  /// This is the encoding primarily used in Sema.  However, in diagnostics,
+  /// Sema uses \c getSourceIndex instead.
+  unsigned getASTIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    assert(Idx >= 1 + HasThis &&
+           "stored index must be base-1 and not specify C++ implicit this");
+    return Idx - 1 - HasThis;
+  }
+
+  /// Get the parameter index as it would normally be encoded at the LLVM level
+  /// of representation: zero-origin including any C++ implicit this parameter.
+  ///
+  /// This is the encoding primarily used in CodeGen.
+  unsigned getLLVMIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    assert(Idx >= 1 && "stored index must be base-1");
+    return Idx - 1;
+  }
+
+  bool operator==(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx == I.Idx;
+  }
+  bool operator!=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx != I.Idx;
+  }
+  bool operator<(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx < I.Idx;
+  }
+  bool operator>(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx > I.Idx;
+  }
+  bool operator<=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx <= I.Idx;
+  }
+  bool operator>=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx >= I.Idx;
+  }
+};
+
 #include "clang/AST/Attrs.inc"
 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const Attr *At) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(At),
                   DiagnosticsEngine::ak_attr);
   return DB;
 }

cfe/trunk/include/clang/Basic/Attr.td

class NamedArgument<string name, bit opt = 0, bit fake = 0> : Argument<name,
                                                                           opt,
                                                                           fake>;
 class TypeArgument<string name, bit opt = 0> : Argument<name, opt>;
 class UnsignedArgument<string name, bit opt = 0> : Argument<name, opt>;
 class VariadicUnsignedArgument<string name> : Argument<name, 1>;
 class VariadicExprArgument<string name> : Argument<name, 1>;
 class VariadicStringArgument<string name> : Argument<name, 1>;
 
+// Like VariadicUnsignedArgument except values are ParamIdx.
+class VariadicParamIdxArgument<string name> : Argument<name, 1>;
+
+// Like VariadicParamIdxArgument but for a single function parameter index.
+class ParamIdxArgument<string name, bit opt = 0> : Argument<name, opt>;
+
 // A version of the form major.minor[.subminor].
 class VersionArgument<string name, bit opt = 0> : Argument<name, opt>;
 
 // This one's a doozy, so it gets its own special type
 // It can be an unsigned integer, or a type. Either can
 // be dependent.
 class AlignedArgument<string name, bit opt = 0> : Argument<name, opt>;
 
let Accessors = [Accessor<"alwaysXRayInstrument",
                    Accessor<"neverXRayInstrument",
                      [Clang<"xray_never_instrument">]>];
   let Documentation = [XRayDocs];
 }
 
 def XRayLogArgs : InheritableAttr {
   let Spellings = [Clang<"xray_log_args">];
   let Subjects = SubjectList<[Function, ObjCMethod]>;
+  // This argument is a count not an index, so it has the same encoding (base
+  // 1 including C++ implicit this parameter) at the source and LLVM levels of
+  // representation, so ParamIdxArgument is inappropriate.  It is never used
+  // at the AST level of representation, so it never needs to be adjusted not
+  // to include any C++ implicit this parameter.  Thus, we just store it and
+  // use it as an unsigned that never needs adjustment.
   let Args = [UnsignedArgument<"ArgumentCount">];
   let Documentation = [XRayDocs];
 }
 
 def TLSModel : InheritableAttr {
   let Spellings = [GCC<"tls_model">];
   let Subjects = SubjectList<[TLSVar], ErrorDiag>;
   let Args = [StringArgument<"Model">];
def EmptyBases : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
   let Spellings = [Declspec<"empty_bases">];
   let Subjects = SubjectList<[CXXRecord]>;
   let Documentation = [EmptyBasesDocs];
 }
 
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
   let Subjects = SubjectList<[Function]>;
-  let Args = [IntArgument<"ElemSizeParam">, IntArgument<"NumElemsParam", 1>];
+  let Args = [ParamIdxArgument<"ElemSizeParam">,
+              ParamIdxArgument<"NumElemsParam", /*opt*/ 1>];
   let TemplateDependent = 1;
   let Documentation = [AllocSizeDocs];
 }
 
 def EnableIf : InheritableAttr {
   // Does not have a [[]] spelling because this attribute requires the ability
   // to parse function arguments but the attribute is not written in the type
   // position.
def Format : InheritableAttr {
   let Args = [IdentifierArgument<"Type">, IntArgument<"FormatIdx">,
               IntArgument<"FirstArg">];
   let Subjects = SubjectList<[ObjCMethod, Block, HasFunctionProto]>;
   let Documentation = [FormatDocs];
 }
 
 def FormatArg : InheritableAttr {
   let Spellings = [GCC<"format_arg">];
-  let Args = [IntArgument<"FormatIdx">];
+  let Args = [ParamIdxArgument<"FormatIdx">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto]>;
   let Documentation = [Undocumented];
 }
 
 def GNUInline : InheritableAttr {
   let Spellings = [GCC<"gnu_inline">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
def NoSplitStack : InheritableAttr {
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [NoSplitStackDocs];
 }
 
 def NonNull : InheritableParamAttr {
   let Spellings = [GCC<"nonnull">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto, ParmVar], WarnDiag,
                              "functions, methods, and parameters">;
-  let Args = [VariadicUnsignedArgument<"Args">];
-  let AdditionalMembers =
-[{bool isNonNull(unsigned idx) const {
+  let Args = [VariadicParamIdxArgument<"Args">];
+  let AdditionalMembers = [{
+    bool isNonNull(unsigned IdxAST) const {
       if (!args_size())
         return true;
-    for (const auto &V : args())
-      if (V == idx)
-        return true;
-    return false;
-  } }];
+      return args_end() != std::find_if(
+          args_begin(), args_end(),
+          [=](const ParamIdx &Idx) { return Idx.getASTIndex() == IdxAST; });
+    }
+  }];
   // FIXME: We should merge duplicates into a single nonnull attribute.
   let InheritEvenIfAlreadyPresent = 1;
   let Documentation = [NonNullDocs];
 }
 
 def ReturnsNonNull : InheritableAttr {
   let Spellings = [GCC<"returns_nonnull">];
   let Subjects = SubjectList<[ObjCMethod, Function]>;
def AssumeAligned : InheritableAttr {
   let Subjects = SubjectList<[ObjCMethod, Function]>;
   let Args = [ExprArgument<"Alignment">, ExprArgument<"Offset", 1>];
   let Documentation = [AssumeAlignedDocs];
 }
 
 def AllocAlign : InheritableAttr {
   let Spellings = [GCC<"alloc_align">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Args = [IntArgument<"ParamIndex">];
+  let Args = [ParamIdxArgument<"ParamIndex">];
   let Documentation = [AllocAlignDocs];
 }
 
 def NoReturn : InheritableAttr {
   let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
   // FIXME: Does GCC allow this on the function instead?
   let Documentation = [Undocumented];
 }
def Ownership : InheritableAttr {
   let AdditionalMembers = [{
     enum OwnershipKind { Holds, Returns, Takes };
     OwnershipKind getOwnKind() const {
       return isHolds() ? Holds :
              isTakes() ? Takes :
              Returns;
     }
   }];
-  let Args = [IdentifierArgument<"Module">, VariadicUnsignedArgument<"Args">];
+  let Args = [IdentifierArgument<"Module">,
+              VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
   let Documentation = [Undocumented];
 }
 
 def Packed : InheritableAttr {
   let Spellings = [GCC<"packed">];
 //  let Subjects = [Tag, Field];
   let Documentation = [Undocumented];

 
 // Type safety attributes for `void *' pointers and type tags.
 
 def ArgumentWithTypeTag : InheritableAttr {
   let Spellings = [Clang<"argument_with_type_tag">,
                    Clang<"pointer_with_type_tag">];
   let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;
   let Args = [IdentifierArgument<"ArgumentKind">,
-              UnsignedArgument<"ArgumentIdx">,
-              UnsignedArgument<"TypeTagIdx">,
+              ParamIdxArgument<"ArgumentIdx">,
+              ParamIdxArgument<"TypeTagIdx">,
               BoolArgument<"IsPointer", /*opt*/0, /*fake*/1>];
   let Documentation = [ArgumentWithTypeTagDocs, PointerWithTypeTagDocs];
 }
 
 def TypeTagForDatatype : InheritableAttr {
   let Spellings = [Clang<"type_tag_for_datatype">];
   let Args = [IdentifierArgument<"ArgumentKind">,
               TypeArgument<"MatchingCType">,

cfe/trunk/lib/AST/ExprConstant.cpp

 ///
 /// This expects the given CallExpr to be a call to a function with an
 /// alloc_size attribute.
 static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
                                             const CallExpr *Call,
                                             llvm::APInt &Result) {
   const AllocSizeAttr *AllocSize = getAllocSizeAttr(Call);
 
-  // alloc_size args are 1-indexed, 0 means not present.
-  assert(AllocSize && AllocSize->getElemSizeParam() != 0);
-  unsigned SizeArgNo = AllocSize->getElemSizeParam() - 1;
+  assert(AllocSize && AllocSize->elemSizeParam().isValid());
+  unsigned SizeArgNo = AllocSize->elemSizeParam().getASTIndex();
   unsigned BitsInSizeT = Ctx.getTypeSize(Ctx.getSizeType());
   if (Call->getNumArgs() <= SizeArgNo)
     return false;
 
   auto EvaluateAsSizeT = [&](const Expr *E, APSInt &Into) {
     if (!E->EvaluateAsInt(Into, Ctx, Expr::SE_AllowSideEffects))
       return false;
     if (Into.isNegative() || !Into.isIntN(BitsInSizeT))
       return false;
     Into = Into.zextOrSelf(BitsInSizeT);
     return true;
   };
 
   APSInt SizeOfElem;
   if (!EvaluateAsSizeT(Call->getArg(SizeArgNo), SizeOfElem))
     return false;
 
-  if (!AllocSize->getNumElemsParam()) {
+  if (!AllocSize->numElemsParam().isValid()) {
     Result = std::move(SizeOfElem);
     return true;
   }
 
   APSInt NumberOfElems;
-  // Argument numbers start at 1
-  unsigned NumArgNo = AllocSize->getNumElemsParam() - 1;
+  unsigned NumArgNo = AllocSize->numElemsParam().getASTIndex();
   if (!EvaluateAsSizeT(Call->getArg(NumArgNo), NumberOfElems))
     return false;
 
   bool Overflow;
   llvm::APInt BytesAvailable = SizeOfElem.umul_ov(NumberOfElems, Overflow);
   if (Overflow)
     return false;
 

cfe/trunk/lib/CodeGen/CGCall.cpp

if (TargetDecl) {
     if (TargetDecl->hasAttr<ReturnsNonNullAttr>())
       RetAttrs.addAttribute(llvm::Attribute::NonNull);
     if (TargetDecl->hasAttr<AnyX86NoCallerSavedRegistersAttr>())
       FuncAttrs.addAttribute("no_caller_saved_registers");
 
     HasOptnone = TargetDecl->hasAttr<OptimizeNoneAttr>();
     if (auto *AllocSize = TargetDecl->getAttr<AllocSizeAttr>()) {
       Optional<unsigned> NumElemsParam;
-      // alloc_size args are base-1, 0 means not present.
-      if (unsigned N = AllocSize->getNumElemsParam())
-        NumElemsParam = N - 1;
-      FuncAttrs.addAllocSizeAttr(AllocSize->getElemSizeParam() - 1,
+      if (AllocSize->numElemsParam().isValid())
+        NumElemsParam = AllocSize->numElemsParam().getLLVMIndex();
+      FuncAttrs.addAllocSizeAttr(AllocSize->elemSizeParam().getLLVMIndex(),
                                  NumElemsParam);
     }
   }
 
   ConstructDefaultFnAttrList(Name, HasOptnone, AttrOnCallSite, FuncAttrs);
 
   if (CodeGenOpts.EnableSegmentedStacks &&
       !(TargetDecl && TargetDecl->hasAttr<NoSplitStackAttr>()))
if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
         OffsetValue = EmitScalarExpr(Offset);
 
       llvm::Value *Alignment = EmitScalarExpr(AA->getAlignment());
       llvm::ConstantInt *AlignmentCI = cast<llvm::ConstantInt>(Alignment);
       EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
                               OffsetValue);
     } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
       llvm::Value *ParamVal =
-          CallArgs[AA->getParamIndex() - 1].RV.getScalarVal();
+          CallArgs[AA->paramIndex().getLLVMIndex()].RV.getScalarVal();
       EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
     }
   }
 
   return Ret;
 }
 
 CGCallee CGCallee::prepareConcreteCallee(CodeGenFunction &CGF) const {

cfe/trunk/lib/Sema/SemaChecking.cpp

for (const auto *NonNull : FDecl->specific_attrs<NonNullAttr>()) {
       if (!NonNull->args_size()) {
         // Easy case: all pointer arguments are nonnull.
         for (const auto *Arg : Args)
           if (S.isValidPointerAttrType(Arg->getType()))
             CheckNonNullArgument(S, Arg, CallSiteLoc);
         return;
       }
 
-      for (unsigned Val : NonNull->args()) {
-        if (Val >= Args.size())
+      for (const ParamIdx &Idx : NonNull->args()) {
+        unsigned IdxAST = Idx.getASTIndex();
+        if (IdxAST >= Args.size())
           continue;
         if (NonNullArgs.empty())
           NonNullArgs.resize(Args.size());
-        NonNullArgs.set(Val);
+        NonNullArgs.set(IdxAST);
       }
     }
   }
 
   if (FDecl && (isa<FunctionDecl>(FDecl) || isa<ObjCMethodDecl>(FDecl))) {
     // Handle the nonnull attribute on the parameters of the
     // function/method.
     ArrayRef<ParmVarDecl*> parms;
case Stmt::DeclRefExprClass: {
     return SLCT_NotALiteral;
   }
 
   case Stmt::CallExprClass:
   case Stmt::CXXMemberCallExprClass: {
     const CallExpr *CE = cast<CallExpr>(E);
     if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
       if (const FormatArgAttr *FA = ND->getAttr<FormatArgAttr>()) {
-        unsigned ArgIndex = FA->getFormatIdx();
-        if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
-          if (MD->isInstance())
-            --ArgIndex;
-        const Expr *Arg = CE->getArg(ArgIndex - 1);
-
+        const Expr *Arg = CE->getArg(FA->formatIdx().getASTIndex());
         return checkFormatStringExpr(S, Arg, Args,
                                      HasVAListArg, format_idx, firstDataArg,
                                      Type, CallType, InFunctionCall,
                                      CheckedVarArgs, UncoveredArg, Offset);
       } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
         unsigned BuiltinID = FD->getBuiltinID();
         if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
             BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {
           const Expr *Arg = CE->getArg(0);
           return checkFormatStringExpr(S, Arg, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
                                        InFunctionCall, CheckedVarArgs,
                                        UncoveredArg, Offset);
         }
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCMessageExprClass: {
     const auto *ME = cast<ObjCMessageExpr>(E);
     if (const auto *ND = ME->getMethodDecl()) {
       if (const auto *FA = ND->getAttr<FormatArgAttr>()) {
-        unsigned ArgIndex = FA->getFormatIdx();
-        const Expr *Arg = ME->getArg(ArgIndex - 1);
+        const Expr *Arg = ME->getArg(FA->formatIdx().getASTIndex());
         return checkFormatStringExpr(
             S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
             CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
       }
     }
 
     return SLCT_NotALiteral;
   }
if (getCurFunction() &&
         unsigned ParamNo = std::distance(FD->param_begin(), ParamIter);
 
         for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
           if (!NonNull->args_size()) {
               ComplainAboutNonnullParamOrCall(NonNull);
               return;
           }
 
-          for (unsigned ArgNo : NonNull->args()) {
-            if (ArgNo == ParamNo) {
+          for (const ParamIdx &ArgNo : NonNull->args()) {
+            if (ArgNo.getASTIndex() == ParamNo) {
               ComplainAboutNonnullParamOrCall(NonNull);
               return;
             }
           }
         }
       }
     }
   }

 
 void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
                                     const ArrayRef<const Expr *> ExprArgs,
                                     SourceLocation CallSiteLoc) {
   const IdentifierInfo *ArgumentKind = Attr->getArgumentKind();
   bool IsPointerAttr = Attr->getIsPointer();
 
   // Retrieve the argument representing the 'type_tag'.
-  if (Attr->getTypeTagIdx() >= ExprArgs.size()) {
-    // Add 1 to display the user's specified value.
+  unsigned TypeTagIdxAST = Attr->typeTagIdx().getASTIndex();
+  if (TypeTagIdxAST >= ExprArgs.size()) {
     Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
-        << 0 << Attr->getTypeTagIdx() + 1;
+        << 0 << Attr->typeTagIdx().getSourceIndex();
     return;
   }
-  const Expr *TypeTagExpr = ExprArgs[Attr->getTypeTagIdx()];
+  const Expr *TypeTagExpr = ExprArgs[TypeTagIdxAST];
   bool FoundWrongKind;
   TypeTagData TypeInfo;
   if (!GetMatchingCType(ArgumentKind, TypeTagExpr, Context,
                         TypeTagForDatatypeMagicValues.get(),
                         FoundWrongKind, TypeInfo)) {
     if (FoundWrongKind)
       Diag(TypeTagExpr->getExprLoc(),
            diag::warn_type_tag_for_datatype_wrong_kind)
         << TypeTagExpr->getSourceRange();
     return;
   }
 
   // Retrieve the argument representing the 'arg_idx'.
-  if (Attr->getArgumentIdx() >= ExprArgs.size()) {
-    // Add 1 to display the user's specified value.
+  unsigned ArgumentIdxAST = Attr->argumentIdx().getASTIndex();
+  if (ArgumentIdxAST >= ExprArgs.size()) {
     Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
-        << 1 << Attr->getArgumentIdx() + 1;
+        << 1 << Attr->argumentIdx().getSourceIndex();
     return;
   }
-  const Expr *ArgumentExpr = ExprArgs[Attr->getArgumentIdx()];
+  const Expr *ArgumentExpr = ExprArgs[ArgumentIdxAST];
   if (IsPointerAttr) {
     // Skip implicit cast of pointer to `void *' (as a function argument).
     if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(ArgumentExpr))
       if (ICE->getType()->isVoidPointerType() &&
           ICE->getCastKind() == CK_BitCast)
         ArgumentExpr = ICE->getSubExpr();
   }
   QualType ArgumentType = ArgumentExpr->getType();

cfe/trunk/lib/Sema/SemaDecl.cpp

if (!FD->hasAttr<FormatAttr>())
                                              Name->isStr("vasprintf") ? 0 : 3,
                                              FD->getLocation()));
   }
 
   if (Name->isStr("__CFStringMakeConstantString")) {
     // We already have a __builtin___CFStringMakeConstantString,
     // but builds that use -fno-constant-cfstrings don't go through that.
     if (!FD->hasAttr<FormatArgAttr>())
-      FD->addAttr(FormatArgAttr::CreateImplicit(Context, 1,
+      FD->addAttr(FormatArgAttr::CreateImplicit(Context, ParamIdx(1, FD),
                                                 FD->getLocation()));
   }
 }
 
 TypedefDecl *Sema::ParseTypedefDecl(Scope *S, Declarator &D, QualType T,
                                     TypeSourceInfo *TInfo) {
   assert(D.getIdentifier() && "Wrong callback for declspec without declarator");
   assert(!T.isNull() && "GetTypeForDeclarator() returned null type");

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

 
 /// \brief Check if IdxExpr is a valid parameter index for a function or
 /// instance method D.  May output an error.
 ///
 /// \returns true if IdxExpr is a valid index.
 template <typename AttrInfo>
 static bool checkFunctionOrMethodParameterIndex(
     Sema &S, const Decl *D, const AttrInfo &AI, unsigned AttrArgNum,
-    const Expr *IdxExpr, uint64_t &Idx, bool AllowImplicitThis = false) {
+    const Expr *IdxExpr, ParamIdx &Idx, bool CanIndexImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
   // Parameters are counted from one.
   bool HP = hasFunctionProto(D);
   bool HasImplicitThisParam = isInstanceMethod(D);
   bool IV = HP && isFunctionOrMethodVariadic(D);
   unsigned NumParams =
       (HP ? getFunctionOrMethodNumParams(D) : 0) + HasImplicitThisParam;
 
   llvm::APSInt IdxInt;
   if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent() ||
       !IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
     S.Diag(getAttrLoc(AI), diag::err_attribute_argument_n_type)
       << getAttrName(AI) << AttrArgNum << AANT_ArgumentIntegerConstant
       << IdxExpr->getSourceRange();
     return false;
   }
 
-  Idx = IdxInt.getLimitedValue();
-  if (Idx < 1 || (!IV && Idx > NumParams)) {
+  Idx = ParamIdx(IdxInt.getLimitedValue(UINT_MAX), D);
+  unsigned IdxSource = Idx.getSourceIndex();
+  if (IdxSource < 1 || (!IV && IdxSource > NumParams)) {
     S.Diag(getAttrLoc(AI), diag::err_attribute_argument_out_of_bounds)
         << getAttrName(AI) << AttrArgNum << IdxExpr->getSourceRange();
     return false;
   }
-  Idx--; // Convert to zero-based.
-  if (HasImplicitThisParam && !AllowImplicitThis) {
-    if (Idx == 0) {
+  if (HasImplicitThisParam && !CanIndexImplicitThis) {
+    if (IdxSource == 1) {
       S.Diag(getAttrLoc(AI),
              diag::err_attribute_invalid_implicit_this_argument)
           << getAttrName(AI) << IdxExpr->getSourceRange();
       return false;
     }
-    --Idx;
   }
 
   return true;
 }
 
 /// \brief Check if the argument \p ArgNum of \p Attr is a ASCII string literal.
 /// If not emit an error and return false. If the argument is an identifier it
 /// will emit an error with a fixit hint and treat it as if it was a string

 
 /// \brief Checks to be sure that the given parameter number is in bounds, and
 /// is an integral type. Will emit appropriate diagnostics if this returns
 /// false.
 ///
 /// AttrArgNo is used to actually retrieve the argument, so it's base-0.
 template <typename AttrInfo>
 static bool checkParamIsIntegerType(Sema &S, const FunctionDecl *FD,
-                                    const AttrInfo &AI, unsigned AttrArgNo,
-                                    bool AllowDependentType = false) {
+                                    const AttrInfo &AI, unsigned AttrArgNo) {
   assert(AI.isArgExpr(AttrArgNo) && "Expected expression argument");
   Expr *AttrArg = AI.getArgAsExpr(AttrArgNo);
-  uint64_t Idx;
+  ParamIdx Idx;
   if (!checkFunctionOrMethodParameterIndex(S, FD, AI, AttrArgNo + 1, AttrArg,
                                            Idx))
     return false;
 
-  const ParmVarDecl *Param = FD->getParamDecl(Idx);
-  if (AllowDependentType && Param->getType()->isDependentType())
-    return true;
+  const ParmVarDecl *Param = FD->getParamDecl(Idx.getASTIndex());
   if (!Param->getType()->isIntegerType() && !Param->getType()->isCharType()) {
     SourceLocation SrcLoc = AttrArg->getLocStart();
     S.Diag(SrcLoc, diag::err_attribute_integers_only)
         << getAttrName(AI) << Param->getSourceRange();
     return false;
   }
   return true;
 }
 
 static void handleAllocSizeAttr(Sema &S, Decl *D, const AttributeList &AL) {
   if (!checkAttributeAtLeastNumArgs(S, AL, 1) ||
       !checkAttributeAtMostNumArgs(S, AL, 2))
     return;
 
   const auto *FD = cast<FunctionDecl>(D);
   if (!FD->getReturnType()->isPointerType()) {
     S.Diag(AL.getLoc(), diag::warn_attribute_return_pointers_only)
         << AL.getName();
     return;
   }
 
   const Expr *SizeExpr = AL.getArgAsExpr(0);
-  int SizeArgNo;
+  int SizeArgNoVal;
   // Parameter indices are 1-indexed, hence Index=1
-  if (!checkPositiveIntArgument(S, AL, SizeExpr, SizeArgNo, /*Index=*/1))
+  if (!checkPositiveIntArgument(S, AL, SizeExpr, SizeArgNoVal, /*Index=*/1))
     return;
+  ParamIdx SizeArgNo(SizeArgNoVal, D);
 
   if (!checkParamIsIntegerType(S, FD, AL, /*AttrArgNo=*/0))
     return;
 
-  // Args are 1-indexed, so 0 implies that the arg was not present
-  int NumberArgNo = 0;
+  ParamIdx NumberArgNo;
   if (AL.getNumArgs() == 2) {
     const Expr *NumberExpr = AL.getArgAsExpr(1);
+    int Val;
     // Parameter indices are 1-based, hence Index=2
-    if (!checkPositiveIntArgument(S, AL, NumberExpr, NumberArgNo,
-                                  /*Index=*/2))
+    if (!checkPositiveIntArgument(S, AL, NumberExpr, Val, /*Index=*/2))
       return;
+    NumberArgNo = ParamIdx(Val, D);
 
     if (!checkParamIsIntegerType(S, FD, AL, /*AttrArgNo=*/1))
       return;
   }
 
   D->addAttr(::new (S.Context)
                  AllocSizeAttr(AL.getRange(), S.Context, SizeArgNo, NumberArgNo,
                                AL.getAttributeSpellingListIndex()));
else
       S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only)
           << AL.getName() << AttrParmRange << TypeRange << 0;
     return false;
   }
   return true;
 }
 
 static void handleNonNullAttr(Sema &S, Decl *D, const AttributeList &AL) {
-  SmallVector<unsigned, 8> NonNullArgs;
+  SmallVector<ParamIdx, 8> NonNullArgs;
   for (unsigned I = 0; I < AL.getNumArgs(); ++I) {
     Expr *Ex = AL.getArgAsExpr(I);
-    uint64_t Idx;
+    ParamIdx Idx;
     if (!checkFunctionOrMethodParameterIndex(S, D, AL, I + 1, Ex, Idx))
       return;
 
     // Is the function argument a pointer type?
-    if (Idx < getFunctionOrMethodNumParams(D) &&
-        !attrNonNullArgCheck(S, getFunctionOrMethodParamType(D, Idx), AL,
-                             Ex->getSourceRange(),
-                             getFunctionOrMethodParamRange(D, Idx)))
+    if (Idx.getASTIndex() < getFunctionOrMethodNumParams(D) &&
+        !attrNonNullArgCheck(
+            S, getFunctionOrMethodParamType(D, Idx.getASTIndex()), AL,
+            getFunctionOrMethodParamRange(D, Idx.getASTIndex())))
       continue;
 
     NonNullArgs.push_back(Idx);
   }
 
   // If no arguments were specified to __attribute__((nonnull)) then all pointer
   // arguments have a nonnull attribute; warn if there aren't any. Skip this
   // check if the attribute came from a macro expansion or a template
   // instantiation.
   if (NonNullArgs.empty() && AL.getLoc().isFileID() &&
       !S.inTemplateInstantiation()) {
     bool AnyPointers = isFunctionOrMethodVariadic(D);
     for (unsigned I = 0, E = getFunctionOrMethodNumParams(D);
          I != E && !AnyPointers; ++I) {
       QualType T = getFunctionOrMethodParamType(D, I);
       if (T->isDependentType() || S.isValidPointerAttrType(T))
         AnyPointers = true;
     }
 
     if (!AnyPointers)
       S.Diag(AL.getLoc(), diag::warn_attribute_nonnull_no_pointers);
   }
 
-  unsigned *Start = NonNullArgs.data();
+  ParamIdx *Start = NonNullArgs.data();
   unsigned Size = NonNullArgs.size();
   llvm::array_pod_sort(Start, Start + Size);
   D->addAttr(::new (S.Context)
                  NonNullAttr(AL.getRange(), S.Context, Start, Size,
                              AL.getAttributeSpellingListIndex()));
 }
 
 static void handleNonNullAttrParameter(Sema &S, ParmVarDecl *D,
                                        const AttributeList &AL) {
   if (AL.getNumArgs() > 0) {
     if (D->getFunctionType()) {
       handleNonNullAttr(S, D, AL);
     } else {
       S.Diag(AL.getLoc(), diag::warn_attribute_nonnull_parm_no_args)
         << D->getSourceRange();
     }
     return;
   }
 
   // Is the argument a pointer type?
   if (!attrNonNullArgCheck(S, D->getType(), AL, SourceRange(),
                            D->getSourceRange()))
     return;
 
   D->addAttr(::new (S.Context)
                  NonNullAttr(AL.getRange(), S.Context, nullptr, 0,
                              AL.getAttributeSpellingListIndex()));
 }
 
 static void handleReturnsNonNullAttr(Sema &S, Decl *D,
                                      const AttributeList &AL) {
   QualType ResultType = getFunctionOrMethodResultType(D);
   SourceRange SR = getFunctionOrMethodResultSourceRange(D);
   if (!attrNonNullArgCheck(S, ResultType, AL, SourceRange(), SR,
                            /* isReturnValue */ true))
void Sema::AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
   D->addAttr(::new (Context)
             AssumeAlignedAttr(AttrRange, Context, E, OE, SpellingListIndex));
 }
 
 void Sema::AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
                              unsigned SpellingListIndex) {
   QualType ResultType = getFunctionOrMethodResultType(D);
 
-  AllocAlignAttr TmpAttr(AttrRange, Context, 0, SpellingListIndex);
+  AllocAlignAttr TmpAttr(AttrRange, Context, ParamIdx(), SpellingListIndex);
   SourceLocation AttrLoc = AttrRange.getBegin();
 
   if (!ResultType->isDependentType() &&
       !isValidPointerAttrType(ResultType, /* RefOkay */ true)) {
     Diag(AttrLoc, diag::warn_attribute_return_pointers_refs_only)
         << &TmpAttr << AttrRange << getFunctionOrMethodResultSourceRange(D);
     return;
   }
 
-  uint64_t IndexVal;
+  ParamIdx Idx;
   const auto *FuncDecl = cast<FunctionDecl>(D);
   if (!checkFunctionOrMethodParameterIndex(*this, FuncDecl, TmpAttr,
-                                           /*AttrArgNo=*/1, ParamExpr,
-                                           IndexVal))
+                                           /*AttrArgNo=*/1, ParamExpr, Idx))
     return;
 
-  QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
+  QualType Ty = getFunctionOrMethodParamType(D, Idx.getASTIndex());
   if (!Ty->isDependentType() && !Ty->isIntegralType(Context)) {
     Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only)
-        << &TmpAttr << FuncDecl->getParamDecl(IndexVal)->getSourceRange();
+        << &TmpAttr
+        << FuncDecl->getParamDecl(Idx.getASTIndex())->getSourceRange();
     return;
   }
 
-  // We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
-  // because that has corrected for the implicit this parameter, and is zero-
-  // based.  The attribute expects what the user wrote explicitly.
-  llvm::APSInt Val;
-  ParamExpr->EvaluateAsInt(Val, Context);
-
-  D->addAttr(::new (Context) AllocAlignAttr(
-      AttrRange, Context, Val.getZExtValue(), SpellingListIndex));
+  D->addAttr(::new (Context)
+                 AllocAlignAttr(AttrRange, Context, Idx, SpellingListIndex));
 }
 
 /// Normalize the attribute, __foo__ becomes foo.
 /// Returns true if normalization was applied.
 static bool normalizeName(StringRef &AttrName) {
   if (AttrName.size() > 4 && AttrName.startswith("__") &&
       AttrName.endswith("__")) {
     AttrName = AttrName.drop_front(2).drop_back(2);
static void handleOwnershipAttr(Sema &S, Decl *D, const AttributeList &AL) {
 
   IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
 
   StringRef ModuleName = Module->getName();
   if (normalizeName(ModuleName)) {
     Module = &S.PP.getIdentifierTable().get(ModuleName);
   }
 
-  SmallVector<unsigned, 8> OwnershipArgs;
+  SmallVector<ParamIdx, 8> OwnershipArgs;
   for (unsigned i = 1; i < AL.getNumArgs(); ++i) {
     Expr *Ex = AL.getArgAsExpr(i);
-    uint64_t Idx;
+    ParamIdx Idx;
     if (!checkFunctionOrMethodParameterIndex(S, D, AL, i, Ex, Idx))
       return;
 
     // Is the function argument a pointer type?
-    QualType T = getFunctionOrMethodParamType(D, Idx);
+    QualType T = getFunctionOrMethodParamType(D, Idx.getASTIndex());
     int Err = -1;  // No error
     switch (K) {
       case OwnershipAttr::Takes:
       case OwnershipAttr::Holds:
         if (!T->isAnyPointerType() && !T->isBlockPointerType())
           Err = 0;
         break;
       case OwnershipAttr::Returns:
for (const auto *I : D->specific_attrs<OwnershipAttr>()) {
       if (I->getOwnKind() != K && I->args_end() !=
           std::find(I->args_begin(), I->args_end(), Idx)) {
         S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible)
           << AL.getName() << I;
         return;
       } else if (K == OwnershipAttr::Returns &&
                  I->getOwnKind() == OwnershipAttr::Returns) {
         // A returns attribute conflicts with any other returns attribute using
-        // a different index. Note, diagnostic reporting is 1-based, but stored
-        // argument indexes are 0-based.
+        // a different index.
         if (std::find(I->args_begin(), I->args_end(), Idx) == I->args_end()) {
           S.Diag(I->getLocation(), diag::err_ownership_returns_index_mismatch)
-              << *(I->args_begin()) + 1;
+              << I->args_begin()->getSourceIndex();
           if (I->args_size())
             S.Diag(AL.getLoc(), diag::note_ownership_returns_index_mismatch)
-                << (unsigned)Idx + 1 << Ex->getSourceRange();
+                << Idx.getSourceIndex() << Ex->getSourceRange();
           return;
         }
       }
     }
     OwnershipArgs.push_back(Idx);
   }
 
-  unsigned* start = OwnershipArgs.data();
-  unsigned size = OwnershipArgs.size();
-  llvm::array_pod_sort(start, start + size);
-
+  ParamIdx *Start = OwnershipArgs.data();
+  unsigned Size = OwnershipArgs.size();
+  llvm::array_pod_sort(Start, Start + Size);
   D->addAttr(::new (S.Context)
-             OwnershipAttr(AL.getLoc(), S.Context, Module, start, size,
+                 OwnershipAttr(AL.getLoc(), S.Context, Module, Start, Size,
                                AL.getAttributeSpellingListIndex()));
 }
 
 static void handleWeakRefAttr(Sema &S, Decl *D, const AttributeList &AL) {
   // Check the attribute arguments.
   if (AL.getNumArgs() > 1) {
     S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments)
       << AL.getName() << 1;
     return;
D->addAttr(::new (S.Context) EnumExtensibilityAttr(
       AL.getRange(), S.Context, ExtensibilityKind,
       AL.getAttributeSpellingListIndex()));
 }
 
 /// Handle __attribute__((format_arg((idx)))) attribute based on
 /// http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
 static void handleFormatArgAttr(Sema &S, Decl *D, const AttributeList &AL) {
   Expr *IdxExpr = AL.getArgAsExpr(0);
-  uint64_t Idx;
+  ParamIdx Idx;
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 1, IdxExpr, Idx))
     return;
 
   // Make sure the format string is really a string.
-  QualType Ty = getFunctionOrMethodParamType(D, Idx);
+  QualType Ty = getFunctionOrMethodParamType(D, Idx.getASTIndex());
 
   bool NotNSStringTy = !isNSStringType(Ty, S.Context);
   if (NotNSStringTy &&
       !isCFStringType(Ty, S.Context) &&
       (!Ty->isPointerType() ||
        !Ty->getAs<PointerType>()->getPointeeType()->isCharType())) {
     S.Diag(AL.getLoc(), diag::err_format_attribute_not)
         << "a string type" << IdxExpr->getSourceRange()
         << getFunctionOrMethodParamRange(D, 0);
     return;
   }
   Ty = getFunctionOrMethodResultType(D);
   if (!isNSStringType(Ty, S.Context) &&
       !isCFStringType(Ty, S.Context) &&
       (!Ty->isPointerType() ||
        !Ty->getAs<PointerType>()->getPointeeType()->isCharType())) {
     S.Diag(AL.getLoc(), diag::err_format_attribute_result_not)
         << (NotNSStringTy ? "string type" : "NSString")
         << IdxExpr->getSourceRange() << getFunctionOrMethodParamRange(D, 0);
     return;
   }
 
-  // We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
-  // because that has corrected for the implicit this parameter, and is zero-
-  // based.  The attribute expects what the user wrote explicitly.
-  llvm::APSInt Val;
-  IdxExpr->EvaluateAsInt(Val, S.Context);
-
-  D->addAttr(::new (S.Context)
-             FormatArgAttr(AL.getRange(), S.Context, Val.getZExtValue(),
-                           AL.getAttributeSpellingListIndex()));
+  D->addAttr(::new (S.Context) FormatArgAttr(
+      AL.getRange(), S.Context, Idx, AL.getAttributeSpellingListIndex()));
 }
 
 enum FormatAttrKind {
   CFStringFormat,
   NSStringFormat,
   StrftimeFormat,
   SupportedFormat,
   IgnoredFormat,

 
 static void handleArgumentWithTypeTagAttr(Sema &S, Decl *D,
                                           const AttributeList &AL) {
   if (!AL.isArgIdent(0)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
       << AL.getName() << /* arg num = */ 1 << AANT_ArgumentIdentifier;
     return;
   }
-  
-  uint64_t ArgumentIdx;
+  ParamIdx ArgumentIdx;
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 2, AL.getArgAsExpr(1),
                                            ArgumentIdx))
     return;
 
-  uint64_t TypeTagIdx;
+  ParamIdx TypeTagIdx;
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 3, AL.getArgAsExpr(2),
                                            TypeTagIdx))
     return;
 
   bool IsPointer = AL.getName()->getName() == "pointer_with_type_tag";
   if (IsPointer) {
     // Ensure that buffer has a pointer type.
-    if (ArgumentIdx >= getFunctionOrMethodNumParams(D) ||
-        !getFunctionOrMethodParamType(D, ArgumentIdx)->isPointerType())
+    unsigned ArgumentIdxAST = ArgumentIdx.getASTIndex();
+    if (ArgumentIdxAST >= getFunctionOrMethodNumParams(D) ||
+        !getFunctionOrMethodParamType(D, ArgumentIdxAST)->isPointerType())
       S.Diag(AL.getLoc(), diag::err_attribute_pointers_only)
           << AL.getName() << 0;
   }
 
   D->addAttr(::new (S.Context) ArgumentWithTypeTagAttr(
       AL.getRange(), S.Context, AL.getArgAsIdent(0)->Ident, ArgumentIdx,
       TypeTagIdx, IsPointer, AL.getAttributeSpellingListIndex()));
 }
static void handleTypeTagForDatatypeAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context)
              TypeTagForDatatypeAttr(AL.getRange(), S.Context, PointerKind,
                                     MatchingCTypeLoc,
                                     AL.getLayoutCompatible(),
                                     AL.getMustBeNull(),
                                     AL.getAttributeSpellingListIndex()));
 }
 
-static void handleXRayLogArgsAttr(Sema &S, Decl *D,
-                                  const AttributeList &AL) {
-  uint64_t ArgCount;
+static void handleXRayLogArgsAttr(Sema &S, Decl *D, const AttributeList &AL) {
+  ParamIdx ArgCount;
 
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 1, AL.getArgAsExpr(0),
                                            ArgCount,
-                                           true /* AllowImplicitThis*/))
+                                           true /* CanIndexImplicitThis */))
     return;
 
-  // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
-  D->addAttr(::new (S.Context)
-                 XRayLogArgsAttr(AL.getRange(), S.Context, ++ArgCount,
-                                 AL.getAttributeSpellingListIndex()));
+  // ArgCount isn't a parameter index [0;n), it's a count [1;n]
+  D->addAttr(::new (S.Context) XRayLogArgsAttr(
+      AL.getRange(), S.Context, ArgCount.getSourceIndex(),
 }
 
 //===----------------------------------------------------------------------===//
 // Checker-specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 static bool isValidSubjectOfNSReturnsRetainedAttribute(QualType QT) {
   return QT->isDependentType() || QT->isObjCRetainableType();

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (!Result.isInvalid())
     S.AddAlignValueAttr(Aligned->getLocation(), New, Result.getAs<Expr>(),
                         Aligned->getSpellingListIndex());
 }
 
 static void instantiateDependentAllocAlignAttr(
     Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
     const AllocAlignAttr *Align, Decl *New) {
   Expr *Param = IntegerLiteral::Create(
-      S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+      S.getASTContext(), llvm::APInt(64, Align->paramIndex().getSourceIndex()),
       S.getASTContext().UnsignedLongLongTy, Align->getLocation());
   S.AddAllocAlignAttr(Align->getLocation(), New, Param,
                       Align->getSpellingListIndex());
 }
 
 static Expr *instantiateDependentFunctionAttrCondition(
     Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
     const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE,
                                     const OwnershipAttr *Att,
                                     ProgramStateRef State) const {
   if (!State)
     return nullptr;
 
   if (Att->getModule() != II_malloc)
     return nullptr;
 
-  OwnershipAttr::args_iterator I = Att->args_begin(), E = Att->args_end();
+  ParamIdx *I = Att->args_begin(), *E = Att->args_end();
   if (I != E) {
-    return MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(), State);
+    return MallocMemAux(C, CE, CE->getArg(I->getASTIndex()), UndefinedVal(),
+                        State);
   }
   return MallocMemAux(C, CE, UnknownVal(), UndefinedVal(), State);
 }
 
 ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
                                             const CallExpr *CE,
                                             const Expr *SizeEx, SVal Init,
                                             ProgramStateRef State,
if (!State)
     return nullptr;
 
   if (Att->getModule() != II_malloc)
     return nullptr;
 
   bool ReleasedAllocated = false;
 
   for (const auto &Arg : Att->args()) {
-    ProgramStateRef StateI = FreeMemAux(C, CE, State, Arg,
-                               Att->getOwnKind() == OwnershipAttr::Holds,
-                               ReleasedAllocated);
+    ProgramStateRef StateI = FreeMemAux(
+        C, CE, State, Arg.getASTIndex(),
+        Att->getOwnKind() == OwnershipAttr::Holds, ReleasedAllocated);
     if (StateI)
       State = StateI;
   }
   return State;
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const CallExpr *CE,

cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

void NonNullParamChecker::checkPreCall(const CallEvent &Call,
   // Merge all non-null attributes
   unsigned NumArgs = Call.getNumArgs();
   llvm::SmallBitVector AttrNonNull(NumArgs);
   for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
     if (!NonNull->args_size()) {
       AttrNonNull.set(0, NumArgs);
       break;
     }
-    for (unsigned Val : NonNull->args()) {
-      if (Val >= NumArgs)
+    for (const ParamIdx &Idx : NonNull->args()) {
+      unsigned IdxAST = Idx.getASTIndex();
+      if (IdxAST >= NumArgs)
         continue;
-      AttrNonNull.set(Val);
+      AttrNonNull.set(IdxAST);
     }
   }
 
   ProgramStateRef state = C.getState();
 
   CallEvent::param_type_iterator TyI = Call.param_type_begin(),
                                  TyE = Call.param_type_end();
 

cfe/trunk/test/CodeGenCXX/alloc-size.cpp

int testIt() {
   // 122 = 4 + 5*4 + 6 + 7*8 + 4*9
   // CHECK: ret i32 122
   return __builtin_object_size(my_malloc<int>(), 0) +
          __builtin_object_size(my_calloc<int>(5), 0) +
          __builtin_object_size(dependent_malloc<6>(), 0) +
          __builtin_object_size(dependent_calloc<7, 8>(), 0) +
          __builtin_object_size(dependent_calloc2<int, 9>(), 0);
 }
+} // namespace templated_alloc_size
+
+class C {
+public:
+  void *my_malloc(int N) __attribute__((alloc_size(2)));
+  void *my_calloc(int N, int M) __attribute__((alloc_size(2, 3)));
+};
+
+// CHECK-LABEL: define i32 @_Z16callMemberMallocv
+int callMemberMalloc() {
+  // CHECK: ret i32 16
+  return __builtin_object_size(C().my_malloc(16), 0);
+}
+
+// CHECK-LABEL: define i32 @_Z16callMemberCallocv
+int callMemberCalloc() {
+  // CHECK: ret i32 32
+  return __builtin_object_size(C().my_calloc(16, 2), 0);
 }

cfe/trunk/test/Misc/ast-dump-attr.cpp

 void TestIdentifier(void *, int)
 __attribute__((pointer_with_type_tag(ident1,1,2)));
 // CHECK: FunctionDecl{{.*}}TestIdentifier
 // CHECK:   ArgumentWithTypeTagAttr{{.*}} pointer_with_type_tag ident1
 
 void TestBool(void *, int)
 __attribute__((pointer_with_type_tag(bool1,1,2)));
 // CHECK: FunctionDecl{{.*}}TestBool
-// CHECK:   ArgumentWithTypeTagAttr{{.*}}pointer_with_type_tag bool1 0 1 IsPointer
+// CHECK:   ArgumentWithTypeTagAttr{{.*}}pointer_with_type_tag bool1 1 2 IsPointer
 
 void TestUnsigned(void *, int)
 __attribute__((pointer_with_type_tag(unsigned1,1,2)));
 // CHECK: FunctionDecl{{.*}}TestUnsigned
-// CHECK:   ArgumentWithTypeTagAttr{{.*}} pointer_with_type_tag unsigned1 0 1
+// CHECK:   ArgumentWithTypeTagAttr{{.*}} pointer_with_type_tag unsigned1 1 2
 
 void TestInt(void) __attribute__((constructor(123)));
 // CHECK:      FunctionDecl{{.*}}TestInt
 // CHECK-NEXT:   ConstructorAttr{{.*}} 123
 
 static int TestString __attribute__((alias("alias1")));
 // CHECK:      VarDecl{{.*}}TestString
 // CHECK-NEXT:   AliasAttr{{.*}} "alias1"

cfe/trunk/test/Sema/attr-ownership.cpp

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+class C {
+  void f(int, int)
+      __attribute__((ownership_returns(foo, 2)))  // expected-note {{declared with index 2 here}}
+      __attribute__((ownership_returns(foo, 3))); // expected-error {{'ownership_returns' attribute index does not match; here it is 3}}
+};

cfe/trunk/test/Sema/attr-print.cpp

 // RUN: %clang_cc1 %s -ast-print | FileCheck %s
 
+// CHECK: void xla(int a) __attribute__((xray_log_args(1)));
+void xla(int a) __attribute__((xray_log_args(1)));
+
 // CHECK: void *as2(int, int) __attribute__((alloc_size(1, 2)));
 void *as2(int, int) __attribute__((alloc_size(1, 2)));
 // CHECK: void *as1(void *, int) __attribute__((alloc_size(2)));
 void *as1(void *, int) __attribute__((alloc_size(2)));
+
+// CHECK: void fmt(int, const char *, ...) __attribute__((format(printf, 2, 3)));
+void fmt(int, const char *, ...) __attribute__((format(printf, 2, 3)));
+
+// CHECK: char *fmta(int, const char *) __attribute__((format_arg(2)));
+char *fmta(int, const char *) __attribute__((format_arg(2)));
+
+// CHECK: void nn(int *, int *) __attribute__((nonnull(1, 2)));
+void nn(int *, int *) __attribute__((nonnull(1, 2)));
+
+// CHECK: int *aa(int i) __attribute__((alloc_align(1)));
+int *aa(int i) __attribute__((alloc_align(1)));
+
+// CHECK: void ownt(int *, int *) __attribute__((ownership_takes(foo, 1, 2)));
+void ownt(int *, int *) __attribute__((ownership_takes(foo, 1, 2)));
+// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
+void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
+// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void ownr(int) __attribute__((ownership_returns(foo, 1)));
+
+// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
+void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
+// CHECK: void pwtt(void *, int) __attribute__((pointer_with_type_tag(foo, 1, 2)));
+void pwtt(void *, int) __attribute__((pointer_with_type_tag(foo, 1, 2)));
+
+class C {
+  // CHECK: void xla(int a) __attribute__((xray_log_args(2)));
+  void xla(int a) __attribute__((xray_log_args(2)));
+
+  // CHECK: void *as2(int, int) __attribute__((alloc_size(2, 3)));
+  void *as2(int, int) __attribute__((alloc_size(2, 3)));
+  // CHECK: void *as1(void *, int) __attribute__((alloc_size(3)));
+  void *as1(void *, int) __attribute__((alloc_size(3)));
+
+  // CHECK: void fmt(int, const char *, ...) __attribute__((format(printf, 3, 4)));
+  void fmt(int, const char *, ...) __attribute__((format(printf, 3, 4)));
+
+  // CHECK: char *fmta(int, const char *) __attribute__((format_arg(3)));
+  char *fmta(int, const char *) __attribute__((format_arg(3)));
+
+  // CHECK: void nn(int *, int *) __attribute__((nonnull(2, 3)));
+  void nn(int *, int *) __attribute__((nonnull(2, 3)));
+
+  // CHECK: int *aa(int i) __attribute__((alloc_align(2)));
+  int *aa(int i) __attribute__((alloc_align(2)));
+
+  // CHECK: void ownt(int *, int *) __attribute__((ownership_takes(foo, 2, 3)));
+  void ownt(int *, int *) __attribute__((ownership_takes(foo, 2, 3)));
+  // CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
+  void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
+  // CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
+  void ownr(int) __attribute__((ownership_returns(foo, 2)));
+
+  // CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
+  void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
+  // CHECK: void pwtt(void *, int) __attribute__((pointer_with_type_tag(foo, 2, 3)));
+  void pwtt(void *, int) __attribute__((pointer_with_type_tag(foo, 2, 3)));
+};

cfe/trunk/test/Sema/error-type-safety.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 #define INT_TAG 42
 
 static const int test_in
     __attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
 
 // Argument index: 1, Type tag index: 2
 void test_bounds_index(...)
     __attribute__((argument_with_type_tag(test, 1, 2)));
 
+// Argument index: 1, Type tag index: 2
+void test_bounds_index_ptr(void *, ...)
+    __attribute__((pointer_with_type_tag(test, 1, 2)));
+
 // Argument index: 3, Type tag index: 1
 void test_bounds_arg_index(...)
     __attribute__((argument_with_type_tag(test, 3, 1)));
 
+class C {
+public:
+  // Argument index: 2, Type tag index: 3
+  void test_bounds_index(...)
+      __attribute__((argument_with_type_tag(test, 2, 3)));
+
+  // Argument index: 2, Type tag index: 3
+  void test_bounds_index_ptr(void *, ...)
+      __attribute__((pointer_with_type_tag(test, 2, 3)));
+
+  // Argument index: 4, Type tag index: 2
+  void test_bounds_arg_index(...)
+      __attribute__((argument_with_type_tag(test, 4, 2)));
+};
+
 void test_bounds()
 {
+  C c;
+
   // Test the boundary edges (ensure no off-by-one) with argument indexing.
   test_bounds_index(1, INT_TAG);
+  c.test_bounds_index(1, INT_TAG);
+  test_bounds_index_ptr(0, INT_TAG);
+  c.test_bounds_index_ptr(0, INT_TAG);
 
-  test_bounds_index(1); // expected-error {{type tag index 2 is greater than the number of arguments specified}}
-  test_bounds_arg_index(INT_TAG, 1); // expected-error {{argument index 3 is greater than the number of arguments specified}}
+  test_bounds_index(1);       // expected-error {{type tag index 2 is greater than the number of arguments specified}}
+  c.test_bounds_index(1);     // expected-error {{type tag index 3 is greater than the number of arguments specified}}
+  test_bounds_index_ptr(0);   // expected-error {{type tag index 2 is greater than the number of arguments specified}}
+  c.test_bounds_index_ptr(0); // expected-error {{type tag index 3 is greater than the number of arguments specified}}
+
+  test_bounds_arg_index(INT_TAG, 1);   // expected-error {{argument index 3 is greater than the number of arguments specified}}
+  c.test_bounds_arg_index(INT_TAG, 1); // expected-error {{argument index 4 is greater than the number of arguments specified}}
 }

cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

public:
     void writePCHWrite(raw_ostream &OS) const override {
       OS << "    " << WritePCHRecord(type, "SA->get" +
                                            std::string(getUpperName()) + "()");
     }
 
     std::string getIsOmitted() const override {
       if (type == "IdentifierInfo *")
         return "!get" + getUpperName().str() + "()";
-      // FIXME: Do this declaratively in Attr.td.
-      if (getAttrName() == "AllocSize")
-        return "0 == get" + getUpperName().str() + "()";
       return "false";
     }
 
     void writeValue(raw_ostream &OS) const override {
       if (type == "FunctionDecl *")
         OS << "\" << get" << getUpperName()
            << "()->getNameInfo().getAsString() << \"";
       else if (type == "IdentifierInfo *")
public:
     }
 
     void writeDump(raw_ostream &OS) const override {
       OS << "    for (const auto &Val : SA->" << RangeName << "())\n";
       OS << "      OS << \" \" << Val;\n";
     }
   };
 
+  class VariadicParamIdxArgument : public VariadicArgument {
+  public:
+    VariadicParamIdxArgument(const Record &Arg, StringRef Attr)
+        : VariadicArgument(Arg, Attr, "ParamIdx") {}
+
+  public:
+    void writeCtorBody(raw_ostream &OS) const override {
+      VariadicArgument::writeCtorBody(OS);
+      OS << "    #ifndef NDEBUG\n"
+         << "    if (" << getLowerName() << "_size()) {\n"
+         << "      bool HasThis = " << getLowerName()
+         << "_begin()->hasThis();\n"
+         << "      for (const auto Idx : " << getLowerName() << "()) {\n"
+         << "        assert(Idx.isValid() && \"ParamIdx must be valid\");\n"
+         << "        assert(HasThis == Idx.hasThis() && "
+         << "\"HasThis must be consistent\");\n"
+         << "      }\n"
+         << "    }\n"
+         << "    #endif\n";
+    }
+
+    void writePCHReadDecls(raw_ostream &OS) const override {
+      OS << "    unsigned " << getUpperName() << "Size = Record.readInt();\n";
+      OS << "    bool " << getUpperName() << "HasThis = " << getUpperName()
+         << "Size ? Record.readInt() : false;\n";
+      OS << "    SmallVector<ParamIdx, 4> " << getUpperName() << ";\n"
+         << "    " << getUpperName() << ".reserve(" << getUpperName()
+         << "Size);\n"
+         << "    for (unsigned i = 0; i != " << getUpperName()
+         << "Size; ++i) {\n"
+         << "      " << getUpperName()
+         << ".push_back(ParamIdx(Record.readInt(), " << getUpperName()
+         << "HasThis));\n"
+         << "    }\n";
+    }
+
+    void writePCHReadArgs(raw_ostream &OS) const override {
+      OS << getUpperName() << ".data(), " << getUpperName() << "Size";
+    }
+
+    void writePCHWrite(raw_ostream &OS) const override {
+      OS << "    Record.push_back(SA->" << getLowerName() << "_size());\n";
+      OS << "    if (SA->" << getLowerName() << "_size())\n"
+         << "      Record.push_back(SA->" << getLowerName()
+         << "_begin()->hasThis());\n";
+      OS << "    for (auto Idx : SA->" << getLowerName() << "())\n"
+         << "      Record.push_back(Idx.getSourceIndex());\n";
+    }
+
+    void writeValueImpl(raw_ostream &OS) const override {
+      OS << "    OS << Val.getSourceIndex();\n";
+    }
+
+    void writeDump(raw_ostream &OS) const override {
+      OS << "    for (auto Idx : SA->" << getLowerName() << "())\n";
+      OS << "      OS << \" \" << Idx.getSourceIndex();\n";
+    }
+  };
+
+  class ParamIdxArgument : public Argument {
+    std::string IdxName;
+
+  public:
+    ParamIdxArgument(const Record &Arg, StringRef Attr)
+        : Argument(Arg, Attr), IdxName(getUpperName()) {}
+
+    void writeDeclarations(raw_ostream &OS) const override {
+      OS << "ParamIdx " << IdxName << ";\n";
+    }
+
+    void writeAccessors(raw_ostream &OS) const override {
+      OS << "\n"
+         << "  ParamIdx " << getLowerName() << "() const {"
+         << " return " << IdxName << "; }\n";
+    }
+
+    void writeCtorParameters(raw_ostream &OS) const override {
+      OS << "ParamIdx " << IdxName;
+    }
+
+    void writeCloneArgs(raw_ostream &OS) const override { OS << IdxName; }
+
+    void writeTemplateInstantiationArgs(raw_ostream &OS) const override {
+      OS << "A->" << getLowerName() << "()";
+    }
+
+    void writeImplicitCtorArgs(raw_ostream &OS) const override {
+      OS << IdxName;
+    }
+
+    void writeCtorInitializers(raw_ostream &OS) const override {
+      OS << IdxName << "(" << IdxName << ")";
+    }
+
+    void writeCtorDefaultInitializers(raw_ostream &OS) const override {
+      OS << IdxName << "()";
+    }
+
+    void writePCHReadDecls(raw_ostream &OS) const override {
+      OS << "    unsigned " << IdxName << "Src = Record.readInt();\n";
+      OS << "    bool " << IdxName << "HasThis = Record.readInt();\n";
+    }
+
+    void writePCHReadArgs(raw_ostream &OS) const override {
+      OS << "ParamIdx(" << IdxName << "Src, " << IdxName << "HasThis)";
+    }
+
+    void writePCHWrite(raw_ostream &OS) const override {
+      OS << "    Record.push_back(SA->" << getLowerName()
+         << "().isValid() ? SA->" << getLowerName()
+         << "().getSourceIndex() : 0);\n";
+      OS << "    Record.push_back(SA->" << getLowerName()
+         << "().isValid() ? SA->" << getLowerName()
+         << "().hasThis() : false);\n";
+    }
+
+    std::string getIsOmitted() const override {
+      return "!" + IdxName + ".isValid()";
+    }
+
+    void writeValue(raw_ostream &OS) const override {
+      OS << "\" << " << IdxName << ".getSourceIndex() << \"";
+    }
+
+    void writeDump(raw_ostream &OS) const override {
+      if (isOptional())
+        OS << "    if (SA->" << getLowerName() << "().isValid())\n  ";
+      OS << "    OS << \" \" << SA->" << getLowerName()
+         << "().getSourceIndex();\n";
+    }
+  };
+
   // Unique the enums, but maintain the original declaration ordering.
   std::vector<StringRef>
   uniqueEnumsInOrder(const std::vector<StringRef> &enums) {
     std::vector<StringRef> uniques;
     SmallDenseSet<StringRef, 8> unique_set;
     for (const auto &i : enums) {
       if (unique_set.insert(i).second)
         uniques.push_back(i);
createArgument(const Record &Arg, StringRef Attr,
   else if (ArgName == "VariadicUnsignedArgument")
     Ptr = llvm::make_unique<VariadicArgument>(Arg, Attr, "unsigned");
   else if (ArgName == "VariadicStringArgument")
     Ptr = llvm::make_unique<VariadicStringArgument>(Arg, Attr);
   else if (ArgName == "VariadicEnumArgument")
     Ptr = llvm::make_unique<VariadicEnumArgument>(Arg, Attr);
   else if (ArgName == "VariadicExprArgument")
     Ptr = llvm::make_unique<VariadicExprArgument>(Arg, Attr);
+  else if (ArgName == "VariadicParamIdxArgument")
+    Ptr = llvm::make_unique<VariadicParamIdxArgument>(Arg, Attr);
+  else if (ArgName == "ParamIdxArgument")
+    Ptr = llvm::make_unique<ParamIdxArgument>(Arg, Attr);
   else if (ArgName == "VersionArgument")
     Ptr = llvm::make_unique<VersionArgument>(Arg, Attr);
 
   if (!Ptr) {
     // Search in reverse order so that the most-derived type is handled first.
     ArrayRef<std::pair<Record*, SMRange>> Bases = Search->getSuperClasses();
     for (const auto &Base : llvm::reverse(Bases)) {
       if ((Ptr = createArgument(Arg, Attr, Base.first)))