[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. @compnerd Thanks for reporting. I will take a look as soon as possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/ https://reviews.llvm.org/D99 ___

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

compnerd added a comment. Herald added a project: All. The sema portions of this change are still causing an issue. Although the revert (rGf95bd18b5faa6a5af4b5786312c373c5b2dce687 ) and the subsequent re-application in

Re: [PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

On 11/6/21 4:36 PM, Nathan Chancellor via Phabricator wrote: nathanchance added a comment. I bisected a crash in the Linux kernel down to the reland commit (and it is not fixed as of https://github.com/llvm/llvm-project/commit/2249ecee8d9a6237f51485bd39f01ba031575987): $ git bisect log

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

nathanchance added a comment. I bisected a crash in the Linux kernel down to the reland commit (and it is not fixed as of https://github.com/llvm/llvm-project/commit/2249ecee8d9a6237f51485bd39f01ba031575987): $ git bisect log # bad: [bdaa181007a46d317a1665acb8234eddeee82815]

Re: [PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

On 11/5/21 1:40 AM, Martin Storsjö via Phabricator wrote: mstorsjo added a comment. I went ahead and reverted this, as it caused crashes when compiling a number of projects. The most reduced testcase is this: $ cat reduced.c void a(*); void a() {} $ clang -c reduced.c -O2 -g A

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

mstorsjo added a comment. I went ahead and reverted this, as it caused crashes when compiling a number of projects. The most reduced testcase is this: $ cat reduced.c void a(*); void a() {} $ clang -c reduced.c -O2 -g A full case (which reduces into this) is this,

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGeb0fa8bfa356: [Clang][Attr] Support btf_type_tag attribute (authored by yonghong-song). Changed prior to commit:

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 384843. yonghong-song added a comment. - Change TypeLoc.isNull() check to !TypeLoc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/ https://reviews.llvm.org/D99 Files:

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1426 + TypeLoc RetTL; + if (!TL.isNull()) { +if (auto FTL = TL.getAs()) dblaikie wrote: > I'm /guessing/ this can be rewritten as: > ``` > if (TL) > ``` > ? (similarly

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie accepted this revision. dblaikie added a comment. Seems unfortunate that attributes on types are only available through TypeLocs rather than through sugar (like if we used a typedef it'd be visible in the type sugar, but not if it's written on the type usage itself) - but that's above

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D99#3108092 , @yonghong-song wrote: >> That sounds reasonable to me, but one possibility would be to change >> createType() and

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. > That sounds reasonable to me, but one possibility would be to change > createType() and getOrCreateType() to take a TypeSourceInfo * rather than a > QualType (because you can go from the TypeSourceInfo * back to the QualType > by calling getType() on it).

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. In D99#3104804 , @yonghong-song wrote: > @aaron.ballman I checked the source. Looks like we can easily get TypeLoc > from TypeSourceInfo, but not from TypeSourceInfo to TypeLoc. But We need > TypeLoc so we can get

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. @aaron.ballman I checked the source. Looks like we can easily get TypeLoc from TypeSourceInfo, but not from TypeSourceInfo to TypeLoc. But We need TypeLoc so we can get attr information and also traverse TypeLoc's.. We might be able to pass TypeSourceInfo in a

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. In D99#3101484 , @yonghong-song wrote: > Just to be sure my understanding is correct. Given an AttributedType node, we > do have a way to get the corresponding Attr, is it right? @aaron.ballman Oh yeah, now I

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. Just to be sure my understanding is correct. Given an AttributedType node, we do have a way to get the corresponding Attr, is it right? @aaron.ballman Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3099988 , @aaron.ballman wrote: > In D99#3097692 , @dblaikie > wrote: > >> In D99#3096124 , >> @aaron.ballman wrote: >>

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. In D99#3097692 , @dblaikie wrote: > In D99#3096124 , @aaron.ballman > wrote: > >> In D99#3095623 , >> @yonghong-song wrote:

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3096124 , @aaron.ballman wrote: > In D99#3095623 , @yonghong-song > wrote: > >>> Ah, yeah, I see what you mean - that does seem sort of unfortunate. Is it >>> possible

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. In D99#3095623 , @yonghong-song wrote: >> Ah, yeah, I see what you mean - that does seem sort of unfortunate. Is it >> possible these attributes could only appear on typedefs and they'd be more >> readily carried

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. > Ah, yeah, I see what you mean - that does seem sort of unfortunate. Is it > possible these attributes could only appear on typedefs and they'd be more > readily carried through that without needing extra typeloc tracking? (sorry > for not having read back

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3093448 , @aaron.ballman wrote: > Attribute bits look good to me, but I'd appreciate if @dblaikie could weigh > in on whether he thinks the CodeGen changes are fine. My concern there is > around whether the changes

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. Attribute bits look good to me, but I'd appreciate if @dblaikie could weigh in on whether he thinks the CodeGen changes are fine. My concern there is around whether the changes to function signatures to accept a `TypeLoc` are reasonable or not.

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. Agreed. If after semantic analysis for a declaration we can check declaration's typeloc chain again, we might be able to give proper warnings. I am happy to implement this as a followup as you suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:6526 + } + + ASTContext = S.Context; yonghong-song wrote: > aaron.ballman wrote: > > Should you also validate that the type is a pointer type? > Looks like we cannot do this. For

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 382518. yonghong-song added a comment. - fix various clang-format and coding style issue suggested by @aaron.ballman Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. All other comments make sense. Will fix them in the next revision. Comment at: clang/lib/Sema/SemaType.cpp:6526 + } + + ASTContext = S.Context; aaron.ballman wrote: > Should you also validate that the type is a pointer type?

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added reviewers: rsmith, dblaikie. aaron.ballman added a comment. Adding some additional reviewers who may have more opinions about the changes in CodeGen. Comment at: clang/include/clang/Basic/AttrDocs.td:2029-2034 +Clang supports the

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 382143. yonghong-song added a comment. - fix clang-format warnings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/ https://reviews.llvm.org/D99 Files: clang/include/clang/Basic/Attr.td

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 382088. yonghong-song added a comment. - removed AttributedBTFType subclass. Instead of using corresponding *TypeLoc to retrieve string argument for btf_type_tag attribute. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. In D99#3078184 , @aaron.ballman wrote: > I reviewed this a bit, but I think it might be heading in a slightly wrong > direction. I think you should be using an `AttributedType` but I don't think > we need to add

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

aaron.ballman added a comment. I reviewed this a bit, but I think it might be heading in a slightly wrong direction. I think you should be using an `AttributedType` but I don't think we need to add `AttributedBTFType` as a subclass to do this. An `AttributedType` holds the type kind

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song added a comment. @aaron.ballman ping. Did you get time to look at this patch? Esp. any comments on this approach with a subclass AttributedBTF vs. putting a StringRef directly in Attributed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 380049. yonghong-song added a subscriber: urnathan. yonghong-song added a comment. - fix a few clang-format issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99/new/

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

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

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

yonghong-song updated this revision to Diff 379772. yonghong-song retitled this revision from "[POC][BTF] support btf_type_tag attribute" to "[Clang][LLVM][Attr] support btf_type_tag attribute". yonghong-song edited the summary of this revision. yonghong-song added a comment. Herald added