Page MenuHomePhabricator

[clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified
ClosedPublic

Authored by fcloutier on Jan 29 2021, 12:51 PM.

Details

Summary

Since it gained a new VariadicExprArgument, AnnotateAttr's printPretty no longer prints back compilable code when the attribute is only passed a string. This is because the comma-printing logic unconditionally prints a comma between the first, fixed argument and the VariadicExprArgument, which is most likely an empty collection.

This diff adds a Comma helper to AttrImpl.inc that prints a comma before an argument if it isn't the first argument. In the process, it simplifies substantially the generation code, and arguably the generated code, too. As usual when printing lists of things, the main insight is that all components should agree on whether you print a comma before or after the item body (here we choose to do it before).

rdar://73742471

Diff Detail

Event Timeline

fcloutier created this revision.Jan 29 2021, 12:51 PM
fcloutier requested review of this revision.Jan 29 2021, 12:51 PM
fcloutier edited the summary of this revision. (Show Details)Jan 29 2021, 12:57 PM
fcloutier updated this revision to Diff 320204.Jan 29 2021, 1:24 PM

Updating the diff using arcanist, which I'm told produces better results. Sorry for the churn!

aaron.ballman added inline comments.Feb 1 2021, 7:15 AM
clang/test/AST/ast-print-attr.c
31

Can you add a second test that shows we properly print the comma? e.g., int fun_annotate2() __attribute__((annotate("annotation one", "annotation two")));

clang/utils/TableGen/ClangAttrEmitter.cpp
2252

One downside to printing the opening paren is that this can't be used in a generic way for generating any comma-separate list. That said, I think this functionality is clean -- perhaps renaming the function from Comma to PrintAttributeArgListElement or something would be an improvement?

fcloutier updated this revision to Diff 320904.Feb 2 2021, 2:20 PM

Address Aaron's feedback

fcloutier added inline comments.Feb 2 2021, 2:22 PM
clang/test/AST/ast-print-attr.c
31

I switched this to use ownership_holds and ownership_returns because annotate has a VariadicExprArgument and it doesn't know how to print expressions (it just prints a pointer value). ownership_holds and ownership_returns are the same attribute, and they have the same bug-triggering configuration (a fixed argument followed by a possibly-empty list):

c++
int *fun_returns() __attribute__((ownership_returns(fun_returns)));
void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));

In the ownership_returns case, without my change, Clang prints __attribute__((ownership_returns(foo, )));.

clang/utils/TableGen/ClangAttrEmitter.cpp
2252

I went with DelimitAttributeArgument.

aaron.ballman accepted this revision.Feb 3 2021, 5:59 AM

Thank you for the fix, the changes LGTM! Do you need me to commit on your behalf?

This revision is now accepted and ready to land.Feb 3 2021, 5:59 AM
This revision was landed with ongoing or failed builds.Feb 3 2021, 11:43 AM
This revision was automatically updated to reflect the committed changes.