This is an archive of the discontinued LLVM Phabricator instance.

[clang][DeclPrinter] Improve AST print of function attributes
Needs ReviewPublic

Authored by strimo378 on Aug 8 2023, 6:52 AM.

Details

Summary

The commit improves the AST print output of function attributes. In the previous version, all attributes were output after the function declaration. This is correct for most attributes but e.g. not for [[noreturn]]. Now it is decided for each attribues if it should be output before or after function declaration. It follows the following rule:

  • All [[...]] attributes are placed before declaration
  • __declspec attributes are placed before declaration as requested by Microsoft ... https://learn.microsoft.com/en-us/cpp/cpp/declspec?view=msvc-170
  • Some keyword are placed after the declaration
  • Without a function body attributes are placed after declaration
  • With a function body attributes are placed before declaration because GCC produces are error and Clang a warning otherwise

Diff Detail

Event Timeline

strimo378 created this revision.Aug 8 2023, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 6:52 AM
strimo378 requested review of this revision.Aug 8 2023, 6:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
strimo378 changed the visibility from "Public (No Login Required)" to "No One".
strimo378 updated this revision to Diff 548227.Aug 8 2023, 7:54 AM

Finish version

strimo378 retitled this revision from [clang][DeclPrinter] Improved AST print of function attributes to [clang][DeclPrinter] Improve AST print of function attributes.Aug 8 2023, 7:57 AM
strimo378 edited the summary of this revision. (Show Details)
strimo378 added a reviewer: aaron.ballman.
strimo378 changed the visibility from "No One" to "Public (No Login Required)".

This is looking to make similar changes to the ones proposed in https://reviews.llvm.org/D141714 -- can you coordinate with the other author so there's only one review that covers both needs?

strimo378 updated this revision to Diff 548487.Aug 9 2023, 12:12 AM
strimo378 edited the summary of this revision. (Show Details)

Using Attr::getSyntax function (instead of unnecessarly added Attr::getVariety function)

This patch do not address attributes in variables nor the __declspec case, as D141714 do. His patch looks cleaner and I can surely coordinate with @strimo378 to also fix those cases, but I want an answer to the tblgen question first to see if we decide to drop the tblgen solution or not.

clang/lib/AST/DeclPrinter.cpp
259

@aaron.ballman @erichkeane

Is this enough to describe the position of the attribute kind, or should it go for a tblgen approach, as I did in D141714? This here looks much cleaner than the tblgen IMHO.

283

You can use A->isStandardAttributeSyntax()

286

I am pretty sure this function when called with AttrLocation::BeforeDecl will print an extra space before it prints out the attribute. Unfortunately to fix this it needs to be print into a string and then remove the first space from the generated string. Or do something more clever and patch printPretty so it does not do that.

This patch do not address attributes in variables nor the __declspec case, as D141714 do. His patch looks cleaner and I can surely coordinate with @strimo378 to also fix those cases, but I want an answer to the tblgen question first to see if we decide to drop the tblgen solution or not.

Yeah, the two patches are trying to solve the same problem but for different parts of the language.

In terms of the tablegen approach, what you have in D141714 is along the same lines of what I was thinking of, though I've not had the chance to give it a thorough review yet. It keeps all of the knowledge about the specifics of the attributes in Attr.td instead of putting special per-attribute logic into the declaration printer, which is what I was mostly concerned about.

clang/lib/AST/DeclPrinter.cpp
259

Oofda, I forgot that we model those as keyword attributes. override and final (and other keyword attributes) are a bit trickier than just "before" or "after" because there's a very particular order in which those can be parsed grammatically, especially for functions: https://godbolt.org/z/ad5qecbrj

Thinking out loud: perhaps an approach to solving this is to sort the attributes by source location. So all the before attributes are written in the same order as they were in source, as are all of the after attributes. So long as we're skipping over implicit attributes, this seems like it should keep the order of the attributes correct.

A different alternative that's possibly more work than it's worth: the decl printer could dispatch to a table-genned function to ask the attribute "do you want to be printed now?" based on what part of the declaration we've already printed. This would require the decl printer to know e.g., "I've finished printing the parameter list but haven't yet printed cv qualifiers" and pass that information along to the attribute; individual attributes with special rules could supply special logic in Attr.td to say "the attribute should be printed after the function parameter list and after the cv and ref qualifiers, but before the override/final keywords" while most attributes can hopefully get away with whatever the default behavior is coming out of tablegen.

clang/lib/AST/DeclPrinter.cpp
259

Oofda, I forgot that we model those as keyword attributes. override and final (and other keyword attributes) are a bit trickier than just "before" or "after" because there's a very particular order in which those can be parsed grammatically, especially for functions: https://godbolt.org/z/ad5qecbrj

Thinking out loud: perhaps an approach to solving this is to sort the attributes by source location. So all the before attributes are written in the same order as they were in source, as are all of the after attributes. So long as we're skipping over implicit attributes, this seems like it should keep the order of the attributes correct.

This may work in cases where clang is able to find the SourceLocation of the Decl. My concern is when clang is *not* able to do it. When analyzing Linux sourcecode there are cases where this happen, and I have to fallback to AST dumping. The only way I see this working is by changing the parser for it to mark the attribute and its position. This may be something for the future, but the presented fix solves the issue for the cases we have already found.

A different alternative that's possibly more work than it's worth: the decl printer could dispatch to a table-genned function to ask the attribute "do you want to be printed now?" based on what part of the declaration we've already printed. This would require the decl printer to know e.g., "I've finished printing the parameter list but haven't yet printed cv qualifiers" and pass that information along to the attribute; individual attributes with special rules could supply special logic in Attr.td to say "the attribute should be printed after the function parameter list and after the cv and ref qualifiers, but before the override/final keywords" while most attributes can hopefully get away with whatever the default behavior is coming out of tablegen.

Sounds a lot of work :). Wouldn't my previous suggestion be simpler?

strimo378 edited the summary of this revision. (Show Details)Aug 9 2023, 7:11 PM

This patch do not address attributes in variables nor the __declspec case, as D141714 do. His patch looks cleaner and I can surely coordinate with @strimo378 to also fix those cases, but I want an answer to the tblgen question first to see if we decide to drop the tblgen solution or not.

The patch is intended to only address function attributes to keep it simple and the test cases focused. Most important is that the AttrLocation is introduced and functionality can be easily extended to other decl types in subsequent patches. I did some work to address variable and record attributes but I think that I did not consider all cases for variables as you did.

__declspec should be always placed left?

clang/lib/AST/DeclPrinter.cpp
259

Currently, I would not go for a tblgen approach since we do not know all factors that influence the position. The current logic is relatively simple. I don't know how many additional exceptions are required.

I know for now that the position is influence by the target (function, record, field, parameter), attribute type and attribute syntax. To make matters worse, some attributes can be used for different targets. E.g. FinalAttr can be used for methods and records.

I suggest to keep things simple, stay on a function approach and see how complexity is evolving with future patches. Then we have the best knowledge to find a good tblgen solution.

259

Thinking out loud: perhaps an approach to solving this is to sort the attributes by source location. So all the before attributes are written in the same order as they were in source, as are all of the after attributes. So long as we're skipping over implicit attributes, this seems like it should keep the order of the attributes correct.

I also thought about that approach to decided on the location if it is placed before or after. I do a lot of AST modification where the location is lost or wrong. So for me that would not work in general.

283

Yes, ok

286

Yes, you are fully right. The goal was to keep the patch simple.

To address the space problem, I think the best way would be to extend tblgen and add an optional bool parameter to printPritty function that allows suppressing the extra space. That could be addressed in a subsequent patch.

strimo378 updated this revision to Diff 548916.Aug 10 2023, 1:25 AM

Added more test cases and considering __declspec

strimo378 added inline comments.Aug 10 2023, 1:29 AM
clang/lib/AST/DeclPrinter.cpp
283

I checked isStandardAttributeSyntax(). I cannot use it since it also includes the alignas keyword for some strange reason.

strimo378 edited the summary of this revision. (Show Details)Aug 10 2023, 1:31 AM