Re: [PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

On 3/16/22 10:32 AM, Sterling Augustine via Phabricator wrote: saugustine added a comment. This change ends up leaving some unhandled enum with switches inside lldb, and it isn't obvious to me how to fix them. Can you take a quick look?

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

saugustine added a comment. This change ends up leaving some unhandled enum with switches inside lldb, and it isn't obvious to me how to fix them. Can you take a quick look? llvm-project/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4098:11: error: enumeration value

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

This revision was automatically updated to reflect the committed changes. Closed by commit rG3251ba2d0fcf: [Attr] Fix a btf_type_tag AST generation (authored by yonghong-song). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 415638. yonghong-song added a comment. - change TransformBTFTagAttributedType() implementation with a simple llvm_unreachable() message. I tested linux kernel and its selftests/bpf compilation with clang and found the only TreeTransformation function

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:6872 + const BTFTagAttributedType *oldType = TL.getTypePtr(); + StringRef Tag = oldType->getTag(); + QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:6872 + const BTFTagAttributedType *oldType = TL.getTypePtr(); + StringRef Tag = oldType->getTag(); + QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added a comment. I think we're pretty close; it looks like there's still one unresolved conversation about template instantiation needs and whether we can remove that code or not. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 415326. yonghong-song added a comment. - add one more _Generic test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files: clang/include/clang-c/Index.h

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added inline comments. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void initializeLocal(ASTContext , SourceLocation loc) {} + + QualType getInnerType() const { return getTypePtr()->getWrappedType(); } yonghong-song wrote: > aaron.ballman

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added inline comments. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void initializeLocal(ASTContext , SourceLocation loc) {} + + QualType getInnerType() const { return getTypePtr()->getWrappedType(); } aaron.ballman wrote: > yonghong-song

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void initializeLocal(ASTContext , SourceLocation loc) {} + + QualType getInnerType() const { return getTypePtr()->getWrappedType(); } yonghong-song wrote: > aaron.ballman

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 414872. yonghong-song added a comment. - fix one 'auto' declaration issue - rework on the test with overloadable attribute. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files:

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added inline comments. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void initializeLocal(ASTContext , SourceLocation loc) {} + + QualType getInnerType() const { return getTypePtr()->getWrappedType(); } aaron.ballman wrote: > Do we also

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/TypeLoc.h:923 + void initializeLocal(ASTContext , SourceLocation loc) {} + + QualType getInnerType() const { return getTypePtr()->getWrappedType(); } Do we also need something like this?

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @aaron.ballman @erichkeane Hopefully I addressed your comments. Could you take a look again? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 414267. yonghong-song added a comment. - use BTFTypeTagAttr instead of Attr in PropertiesBase.td Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files:

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 414263. yonghong-song added a comment. - further clang-format fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files: clang/include/clang-c/Index.h

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 414252. yonghong-song added a comment. - clang-format fix - change variable names to be llvm compliant. - remove Attr from BTFTagAttributedTypeLoc as it can be retrieved from the BTFTagAttributedType - Also transform the attribute in TreeTransform -

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @aaron.ballman Thanks for the review! I will address all your comments and will also add tests with `_Generic` and `__attribute__((overloadable))`. Comment at: clang/include/clang/AST/Type.h:4797 + + BTFTagAttributedType(QualType canon,

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added a comment. Thanks for this! One thing I noticed is that we seem to be lacking a lot of Sema tests for the changes now that this is a type attribute. Of particular interest to me would be cases using `_Generic` and `__attribute__((overloadable))` to demonstrate what kind of

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

erichkeane added a comment. 1 comment, otherwise LGTM. I'd like @aaron.ballman to take a look once you've got the attribute transforming as well though. Comment at: clang/lib/Sema/TreeTransform.h:6876 + + BTFTypeTagAttr *Attr = oldType->getAttr(); + QualType result =

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 413995. yonghong-song added a comment. - add a pch test to test serialization with btf_type_tag. - simplify the code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @erichkeane Just posted a patch to address your comments. Could you take a look? I will try to add more tests, esp. serialization and libclang. If you have any suggestions in these two areas (as I am not familiar with them), it would be great! Repository: rG

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 413861. yonghong-song added a comment. - address @erichkeane comments - further TODO: add more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files:

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @erichkeane Thanks for the review. I will try to address your comments as soon as possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added inline comments. Comment at: clang/include/clang/AST/Type.h:4793 + + QualType ModifiedType; + QualType EquivalentType; erichkeane wrote: > I suspect both of these aren't necessary. If the intent here is to just wrap > a single type, I

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

erichkeane added a comment. I only looked at the 'new type' code, and it generally looks correct with the exception of the comments below. Comment at: clang/include/clang/AST/Type.h:4793 + + QualType ModifiedType; + QualType EquivalentType; I suspect both

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @aaron.ballman @erichkeane Just updated the patch with a new implementation, which added a new BTFTagAttributedType instead of reusing existing AttributedType. It is quite complicated and various parts of clang are touched. I mostly just followed what

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 413219. yonghong-song edited the summary of this revision. yonghong-song added a comment. Herald added subscribers: dexonsmith, arphaman, martong. Herald added a project: All. - Create a separate type BTFTagAttributed instead of using Attributed type

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @erichkeane Thanks for suggestion. I will add as an independent type then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 ___

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

erichkeane added a comment. In D120296#3345831 , @yonghong-song wrote: > @aaron.ballman Thanks for suggestion. Agree that Having a new Type is a > better idea. I guess, I will add BTFTagAttributeType which extends > AttributeType to see whether it

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @aaron.ballman Thanks for suggestion. Agree that Having a new Type is a better idea. I guess, I will add BTFTagAttributeType which extends AttributeType to see whether it works or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

aaron.ballman added reviewers: rjmccall, rsmith, erichkeane. aaron.ballman added a comment. In D120296#3343673 , @yonghong-song wrote: > @aaron.ballman ping, did you get time to look at this patch? Sorry, I was in C meetings all last week, still

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song added a comment. @aaron.ballman ping, did you get time to look at this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 ___ cfe-commits

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song updated this revision to Diff 411182. yonghong-song added a comment. fix clang-format issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120296/new/ https://reviews.llvm.org/D120296 Files: clang/include/clang/AST/ASTContext.h

[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

yonghong-song created this revision. yonghong-song added a reviewer: aaron.ballman. yonghong-song requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Current ASTContext.getAttributedType() takes attribute kind, ModifiedType and EquivType as