[Attr] Fix printing of parameter indices in attributes
Needs ReviewPublic

Authored by jdenny on Tue, Feb 13, 10:57 AM.

Details

Summary

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
this parameter, are stored as zero-origin excluding any this
parameter, and were erroneously printing (-ast-print) as the stored
values. This patch fixes those cases by printing re-incremented
values.

An alternative solution is to store only the original values, but that
requires modifying all users. I tried that for nonnull and found that
it required repeating index adjustment logic in many places while this
patch encapsulates that logic in relatively few places.

Another alternative is to store both sets of values, but that would be
less efficient.

This patch also fixes argument_with_type_tag and pointer_with_type_tag
not to print their fake IsPointer arguments.

Diff Detail

jdenny created this revision.Tue, Feb 13, 10:57 AM
aaron.ballman added inline comments.Thu, Feb 15, 9:48 AM
include/clang/Basic/Attr.td
182

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

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

4612

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

test/Sema/attr-print.cpp
1

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

33

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

jdenny added inline comments.Thu, Feb 15, 10:09 AM
include/clang/Basic/Attr.td
182

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

lib/Sema/SemaDeclAttr.cpp
355–357

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

4612

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

test/Sema/attr-print.cpp
1

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

33

Will look into it.

aaron.ballman added inline comments.Thu, Feb 15, 10:16 AM
lib/Sema/SemaDeclAttr.cpp
355–357

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

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

jdenny marked 3 inline comments as done.Thu, Feb 15, 4:12 PM
include/clang/Basic/Attr.td
182

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

Thanks.

aaron.ballman added inline comments.Fri, Feb 16, 7:23 AM
include/clang/Basic/Attr.td
182

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 added inline comments.Fri, Feb 16, 8:55 AM
include/clang/Basic/Attr.td
182

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 added inline comments.Fri, Feb 16, 9:56 AM
include/clang/Basic/Attr.td
182

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 added inline comments.Fri, Feb 16, 10:30 AM
include/clang/Basic/Attr.td
182

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 added inline comments.Fri, Feb 16, 10:32 AM
include/clang/Basic/Attr.td
182

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.