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

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added a comment.

In D111199#3078184 <https://reviews.llvm.org/D111199#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 `AttributedBTFType` as a subclass to do this. An `AttributedType` holds the type kind information in it, but you should be able to access an `AttributedTypeLoc` object to get to the semantic attribute to access the string argument to the attribute. Generally, you'd call `getTypeSourceInfo()` on the declaration to get the type source info for its type, you can call `getTypeLoc()` on that object to get a generic `TypeLoc` object, and then call `getAs<AttributedTypeLoc>()` on that to turn it into an `AttributedTypeLoc` and from there you can call `getAttr()` to get the `Attr *` for the semantic attribute.

Thanks for detailed explanation. This is very useful. I will look into this. I agree with your approach and I myself also have concern with `AttributedBTFType`. I will just use `AttributedType` going forward.



================
Comment at: clang/include/clang/AST/PropertiesBase.td:134
   def ExprRef : SubclassPropertyType<"Expr", StmtRef>;
+def String : PropertyType<"std::string">;
 def TemplateArgument : PropertyType;
----------------
aaron.ballman wrote:
> Is `std::string` really the correct type to be using for AST serialization? Can we use `StringRef` here as well?
I think StringRef may be possible. I will investigate.


================
Comment at: clang/include/clang/AST/TypeLoc.h:896
+    : public InheritingConcreteTypeLoc<AttributedTypeLoc, AttributedBTFTypeLoc,
+                                       AttributedBTFType> {};
+
----------------
aaron.ballman wrote:
> Is this actually needed? I would have assumed the `AttributedTypeLoc` would suffice.
Honestly I am not 100%. But since we will AttributedType and this should not be an issue any more.


================
Comment at: clang/include/clang/Basic/Attr.td:1850
+  let Args = [StringArgument<"BTFTypeTag">];
+  let Documentation = [Undocumented];
+  let LangOpts = [COnly];
----------------
aaron.ballman wrote:
> No new, undocumented attributes please.
Will do in the next revision.


================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:286-287
 
+  void writeString(StringRef Str) { return AddString(Str); }
+
   /// Emit a path.
----------------
aaron.ballman wrote:
> This appears to be unused, can be removed?
Not 100% sure whether this will be used during serialization/deserialization. I will double check.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1152
+      StringRef BTFTypeTag = AT->getBTFTypeTag();
+      if (BTFTypeTag[0]) {
+        llvm::Metadata *Ops[2] = {
----------------
aaron.ballman wrote:
> Should this be checking `!BTFTypeTag.empty()` instead?
Yes, that is true. This probably from old previous revision leftover which I am using a char array and the first '\0' indicating no attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111199/new/

https://reviews.llvm.org/D111199