Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Extend pointer interpretation handling to track explicitness #714

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

jrtc27
Copy link
Member

@jrtc27 jrtc27 commented Sep 2, 2023

We need a new type like DependentPointerType for non-dependent contexts
that can track __capability applied to things like typeof(...) in order
to have a corresponding TypeLoc for the qualifier, otherwise things like
TypeSpecLocFiller get out of sync and, in some cases assert (in other
cases silently use the wrong DeclSpec). However, this then exposes the
fact TypePrinter's PrintingPolicy's SuppressCapabilityQualifier isn't
always able to be set to the right value (e.g. when printing out a
reinterpret_cast to the user, or when dumping the AST), and so we end
up with redundant __capability qualifiers appearing for purecap code in
some cases, and so we need to track alongside the pointer interpretation
whether it was explicit (which also needs care to ensure it doesn't mess
with canonicalisation). Whilst the output is now noisier in cases where
__capability is used in purecap code, this is more faithful to the
source.

The churn in the AST output due to __capability in the source always
being sugar is an unfortunate side-effect, but this should disappear
if llvm/llvm-project#65214 is merged.

Fixes #710

We need a new type like DependentPointerType for non-dependent contexts
that can track __capability applied to things like typeof(...) in order
to have a corresponding TypeLoc for the qualifier, otherwise things like
TypeSpecLocFiller get out of sync and, in some cases assert (in other
cases silently use the wrong DeclSpec). However, this then exposes the
fact TypePrinter's PrintingPolicy's SuppressCapabilityQualifier isn't
always able to be set to the right value (e.g. when printing out a
reinterpret_cast<T> to the user, or when dumping the AST), and so we end
up with redundant __capability qualifiers appearing for purecap code in
some cases, and so we need to track alongside the pointer interpretation
whether it was explicit (which also needs care to ensure it doesn't mess
with canonicalisation). Whilst the output is now noisier in cases where
__capability is used in purecap code, this is more faithful to the
source.

The churn in the AST output due to __capability in the source always
being sugar is an unfortunate side-effect, but this should disappear
if llvm/llvm-project#65214 is merged.

Fixes #710
Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not managed to review the full change yet, but I wonder if we should rename PointerInterpretationKindExplicit to PointerInterpretationKind and the give the current enum a new name to reduce the size of the diff and the length of most new lines?

@jrtc27
Copy link
Member Author

jrtc27 commented Sep 5, 2023

Not managed to review the full change yet, but I wonder if we should rename PointerInterpretationKindExplicit to PointerInterpretationKind and the give the current enum a new name to reduce the size of the diff and the length of most new lines?

Maybe... I was struggling with naming. Also not quite happy with where it is currently, found some more issues with our handling of things (around type printing when you have multiple redundant qualifiers, and around __capability on arrays -- typedef void *T[]; T const x; is void * const x[]; not void *x[const]; like you'd think, so T __capability x[] should be void * __capability;, but actually using that syntax quickly turns into an asserting mess). Needs more love, but this is heading in the right direction, I think.

@arichardson
Copy link
Member

Not managed to review the full change yet, but I wonder if we should rename PointerInterpretationKindExplicit to PointerInterpretationKind and the give the current enum a new name to reduce the size of the diff and the length of most new lines?

Maybe... I was struggling with naming. Also not quite happy with where it is currently, found some more issues with our handling of things (around type printing when you have multiple redundant qualifiers, and around __capability on arrays -- typedef void *T[]; T const x; is void * const x[]; not void *x[const]; like you'd think, so T __capability x[] should be void * __capability;, but actually using that syntax quickly turns into an asserting mess). Needs more love, but this is heading in the right direction, I think.

I agree this is definitely a big improvement over the current state :)

How about PointerInterpretation for the enum+bool struct and keeping PointerInterpretationKind for the enum? Considering explicit can be false I'm not sure it makes that much sense to have it in the name. Hopefully this would just a be simple search+replace without any further need for refactoring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants