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

[Yonghong Song via Phabricator via cfe-commits]

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 think we can have only 1.  Then the fields here are just the Tag, and the WrappedType.
I didn't find WrappedType in clang code base. Maybe I missed something?
But I understand what you mean. Here, unlike AttributedType which needs to deal with attributes which may encode as qualifiers, BTFTagAttributedType won't have this issue so one 'WrappedType' (representing underlying type of the btf_tag attribute) should be enough.


================
Comment at: clang/include/clang/Serialization/ASTRecordReader.h:131
 
+  StringRef readStringRef() { return readString(); }
+
----------------
erichkeane wrote:
> This is a bug.  readString returns a std::string temporary, which creating the StringRef to, it now no longer exists.
Will fix.


================
Comment at: clang/lib/Sema/SemaType.cpp:194
+        std::pair<const BTFTagAttributedType*, const Attr*>;
+    SmallVector<BTFTagTypeAttrPair, 8> AttrsForBTFTagTypes;
+    bool AttrsForBTFTagTypesSorted = true;
----------------
erichkeane wrote:
> This seems pretty absurdly expensive here... Should we instead have the BTFTagAttributedType store its Attr here?
Indeed this is a good idea. This can simplify the code a lot.


================
Comment at: clang/lib/Sema/TreeTransform.h:6872
+  const BTFTagAttributedType *oldType = TL.getTypePtr();
+  StringRef Tag = oldType->getTag();
+  QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
----------------
erichkeane wrote:
> Most of this tree-transform doesn't really do much, since this is a 'C' only type, but otherwise we'd probably want to allow the tag itself to be dependent.  
> 
> We still need this though, since there are other non-template tree-transforms.
> 
> You also might need to make this not contain a `StringRef` based on the serialization issues above.
I will try to do some experiment and simplify this. Indeed this is C and non templates are involved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120296