Page MenuHomePhabricator

[AST] Print correct tag decl for tag specifier
ClosedPublic

Authored by jdenny on Apr 9 2018, 3:16 PM.

Details

Summary

For example, given:

void fn() {
  struct T *p0;
  struct T { int i; } *p1;
}

-ast-print produced:

void fn() {
  struct T { int i; } *p0;
  struct T { int i; } *p1;
}

Compiling that fails with a redefinition error.

Given:

void fn() {
  struct T *p0;
  struct __attribute__((deprecated)) T *p1;
}

-ast-print dropped the attribute.

Details:

For a tag specifier (that is, struct/union/class/enum used as a type
specifier in a declaration) that was also a tag declaration (that is,
first occurrence of the tag) or tag redeclaration (that is, later
occurrence that specifies attributes or a member list), clang printed
the tag specifier as either (1) the full tag definition if one
existed, or (2) the first tag declaration otherwise. Redefinition
errors were sometimes introduced, as in the first example above. Even
when that was impossible because no member list was ever specified,
attributes were sometimes lost, thus changing semantics and
diagnostics, as in the second example above.

This patch fixes a major culprit for these problems. It does so by
creating an ElaboratedType with a new OwnedDecl member wherever an
occurrence of a tag type is a (re)declaration of that tag type.
PrintingPolicy's IncludeTagDefinition used to trigger printing of the
member list, attributes, etc. for a tag specifier by using a tag
(re)declaration selected as described above. Now, it triggers the
same thing except it uses the tag (re)declaration stored in the
OwnedDecl. Of course, other tooling can now make use of the new
OwnedDecl as well.

Also, to be more faithful to the original source, this patch
suppresses printing of attributes inherited from previous
declarations.

Diff Detail

Repository
rC Clang

Event Timeline

jdenny updated this revision to Diff 142673.Apr 16 2018, 12:36 PM

Rebased onto a more recent master.

jbcoe resigned from this revision.Apr 23 2018, 7:21 AM
jdenny updated this revision to Diff 145092.May 3 2018, 2:07 PM
jdenny removed a reviewer: jbcoe.

Rebased. Ping.

rsmith added a comment.May 3 2018, 4:49 PM

TagSpecifierAs expands sizeof(PrintingPolicy) from 8 to 16 bytes. My concern is the comments on PrintingPolicy about how PrintingPolicy is intended to be small. My guess it that 16 bytes is still small enough, but perhaps Richard Smith, who wrote that comment, knows better.

This seems fine. See r270009 for some background for that comment -- we used to store a copy of the LangOptions in the PrintingPolicy (among other things), and copying these objects (including the potentially-large vectors within the LangOptions) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.

include/clang-c/Index.h
4116 ↗(On Diff #145092)

This is not OK: this would be an ABI break for the stable libclang ABI.

lib/AST/DeclPrinter.cpp
181–182

Instead of the changes in this patch, can you address your use case by just changing the if here to if (TD && TD->isThisDeclarationADefinition())?

jdenny added a comment.May 3 2018, 6:43 PM

TagSpecifierAs expands sizeof(PrintingPolicy) from 8 to 16 bytes. My concern is the comments on PrintingPolicy about how PrintingPolicy is intended to be small. My guess it that 16 bytes is still small enough, but perhaps Richard Smith, who wrote that comment, knows better.

This seems fine. See r270009 for some background for that comment -- we used to store a copy of the LangOptions in the PrintingPolicy (among other things), and copying these objects (including the potentially-large vectors within the LangOptions) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.

Makes sense. Thanks.

include/clang-c/Index.h
4116 ↗(On Diff #145092)

Understood. We could keep IncludeTagDefinition for that purpose and just replace its current internal use with TagSpecifierAs.

Out of curiosity, do we know how others are using IncludeTagDefinition?

lib/AST/DeclPrinter.cpp
181–182

It sounds like that would fix the case of duplicated definitions.

It doesn't address the case of dropped or relocated attributes. For example, -ast-print drops the following attribute and thus the deprecation warning:

void fn() {
  struct T *p0;
  struct __attribute__((deprecated)) T *p1;
}

I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

aaron.ballman added inline comments.May 3 2018, 6:48 PM
lib/AST/DeclPrinter.cpp
181–182

I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

This has been a *very* long-standing issue with -ast-print that I'd love to see fixed. The AST printer does not print attributes in the correct locations except by accident, and it oftentimes prints with the wrong syntax, or is missing from places where it should be printed.

Long story short, I agree that this behavior is wrong and needs to be fixed, but it's also going to take a bit of work to get it right.

rsmith added inline comments.May 3 2018, 7:10 PM
lib/AST/DeclPrinter.cpp
181–182

it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

Historically we've viewed -ast-print as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix. However, one of the important and intended uses of the -ast-print mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

It doesn't address the case of dropped or relocated attributes.

Hmm. A big part of the problem here, in my view, is that when we come to print a TagType, we call TagType::getDecl, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type. We could easily fix that (by adding an accessor to reach TagType::decl rather than calling TagType::getDecl()), and perhaps *that* plus the current tracking of whether we're printing the tag declaration that is owned by the current DeclGroup would be enough to get correct displays here.

Another part of the problem is that TagType does not track whether it's referring to its "own" TagDecl (ie, one created when parsing that type) or some pre-existing one, and that affects whether and how attributes should be printed. That's why we need the IncludeTagDefinition flag in the first place; it's our attempt to recreate knowledge about whether the TagType owns its TagDecl. We could consider instead tracking a flag on TagType sugar nodes to model that, which would let us get rid of that flag.

rsmith added inline comments.May 3 2018, 7:21 PM
lib/AST/DeclPrinter.cpp
181–182

Hmm. A big part of the problem here, in my view, is that when we come to print a TagType, we call TagType::getDecl, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type.

Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the ElaboratedType node could track that for us.

jdenny added inline comments.May 3 2018, 9:06 PM
lib/AST/DeclPrinter.cpp
181–182

Historically we've viewed -ast-print as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix.

I've heard that, but it seemed to me that what's good for the latter is likely good for the former.

Still, I don't want to waste time on patches that won't be accepted because they're harder to justify purely for debugging purposes. Is there a better place to explore pretty printing valid source code from a clang AST?

However, one of the important and intended uses of the -ast-print mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

I haven't considered cases where diagnostics are broken by this issue. For diagnostics, is it important to print the exact set of attributes on a particular declaration rather than to print, say, the entire list of attributes from all declarations of the tag? The latter appears to be easier to implement.

Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the ElaboratedType node could track that for us.

My approach was to constrain my changes to the -ast-print implementation as much as possible without changing the AST in order to avoid breaking something else. I found that I could fix a big problem (printing the right tag decl at each specifier) just using the AST as it's already constructed. DeclGroup gives us the info we need. That info just isn't in the most convenient place, as I think you're pointing out. (Also keep in mind that there's a child patch that fixes decl group discovery in some contexts not exercised by this patch.)

If you feel that ElaboratedType is a better way to go, I'll look into it and get back to you later.

jdenny updated this revision to Diff 146145.May 10 2018, 9:26 AM
jdenny edited the summary of this revision. (Show Details)

I've implemented the suggestion to use ElaboratedType. See the last paragraph of the revised summary for details.

There might be trouble for CXPrintingPolicyProperty users. That is, this patch shifts the semantics of IncludeTagDefinition away from the documented semantics and from what the name implies. Then again, both were already misleading for a tag type without a member list (that is, definition).

I see a few possible paths:

  1. Update the documentation for IncludeTagDefinition and assume the new behavior was the expected behavior all along.
  1. #1 and rename IncludeTagDefinition to something like OwnedTagDeclaration.
  1. Add OwnedTagDeclaration alongside IncludeTagDefinition. CXPrintingPolicyProperty users could set either. We'd have to decide which would have precedence. Internally, we would change Decl::printGroup to set OwnedTagDeclaration instead of IncludeTagDefinition, which otherwise would not be used internally.
rsmith added inline comments.May 10 2018, 7:37 PM
include/clang/AST/Type.h
4854–4855

I think it's better to keep this as-is: the "owns a (re)declaration" case is a special case of using an elaborated type keyword.

4904

Are there other types for which it would be meaningful? (If not, we could store and expose this as a TagDecl instead.)

4905–4915

You can use isInherited() on the attribute to find out whether it was written on that declaration or inherited.

jdenny updated this revision to Diff 146351.May 11 2018, 10:28 AM
jdenny marked 10 inline comments as done.
jdenny edited the summary of this revision. (Show Details)

Made the suggested changes. Thanks.

jdenny added inline comments.May 11 2018, 10:34 AM
lib/AST/DeclPrinter.cpp
218

I implemented inherited attribute suppression in this function with the expectation that the test suite would reveal a use case where that's wrong. However, the test suite behaved. If you know of a use case, please let me know.

rsmith accepted this revision.May 11 2018, 12:56 PM

LGTM, thanks!

include/clang/AST/Type.h
4903–4910

Please remove this FIXME. This function *is* returning the right TagDecl. This is the wrong place to call out any infelicities in attribute retention.

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

LGTM, thanks!

Thanks. I'll remove the fixme as you asked.

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

I think we should deprecate that flag, and change it to have no effect. It is an implementation detail of the AST printer and should never have been exposed by libclang in the first place.

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

I think we should deprecate that flag, and change it to have no effect. It is an implementation detail of the AST printer and should never have been exposed by libclang in the first place.

Thanks. I'll address that in a separate patch then.

Closed by commit rC332281: [AST] Print correct tag decl for tag specifier (authored by jdenny, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
jdenny marked an inline comment as done.