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

[Aaron Ballman via Phabricator via cfe-commits]

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 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.



================
Comment at: clang/include/clang/AST/ASTContext.h:1562-1563
 
+  QualType getAttributedBTFType(attr::Kind attrKind, QualType modifiedType,
+                                QualType equivalentType, StringRef BTFTypeTag);
+
----------------



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


================
Comment at: clang/include/clang/AST/Type.h:4779-4783
+  AttributedBTFType(QualType canon, attr::Kind attrKind, QualType modified,
+                    QualType equivalent, StringRef BTFTypeTag)
+      : AttributedType(AttributedBTF, canon, attrKind, modified, equivalent) {
+    this->BTFTypeTag = BTFTypeTag;
+  }
----------------



================
Comment at: clang/include/clang/AST/TypeLoc.h:896
+    : public InheritingConcreteTypeLoc<AttributedTypeLoc, AttributedBTFTypeLoc,
+                                       AttributedBTFType> {};
+
----------------
Is this actually needed? I would have assumed the `AttributedTypeLoc` would suffice.


================
Comment at: clang/include/clang/Basic/Attr.td:1850
+  let Args = [StringArgument<"BTFTypeTag">];
+  let Documentation = [Undocumented];
+  let LangOpts = [COnly];
----------------
No new, undocumented attributes please.


================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:286-287
 
+  void writeString(StringRef Str) { return AddString(Str); }
+
   /// Emit a path.
----------------
This appears to be unused, can be removed?


================
Comment at: clang/lib/AST/ASTContext.cpp:4636-4656
+QualType ASTContext::getAttributedBTFType(attr::Kind attrKind,
+                                          QualType modifiedType,
+                                          QualType equivalentType,
+                                          StringRef BTFTypeTag) {
+  llvm::FoldingSetNodeID id;
+  AttributedBTFType::Profile(id, attrKind, modifiedType, equivalentType,
+                             BTFTypeTag);
----------------



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1152
+      StringRef BTFTypeTag = AT->getBTFTypeTag();
+      if (BTFTypeTag[0]) {
+        llvm::Metadata *Ops[2] = {
----------------
Should this be checking `!BTFTypeTag.empty()` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111199