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

[Aaron Ballman via Phabricator via cfe-commits]

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 type system effects there are. Same for redeclaration behavior or other type system situations.



================
Comment at: clang/include/clang/AST/ASTContext.h:1596
 
+  QualType getBTFTagAttributedType(BTFTypeTagAttr *Attr, QualType Wrapped);
+
----------------



================
Comment at: clang/include/clang/AST/Type.h:4797
+
+  BTFTagAttributedType(QualType canon, QualType wrapped,
+                       BTFTypeTagAttr *attr)
----------------
You should fix the formatting issue, but also, the parameter names don't match the usual naming conventions (please don't replace `attr` with `Attr` though; that's a type name, so go with something else like `BTFAttr`).


================
Comment at: clang/include/clang/AST/Type.h:4804
+  QualType getWrappedType() const { return WrappedType; }
+  BTFTypeTagAttr *getAttr() const { return Attr; }
+
----------------
Should this be returning a `const BTFTypeTagAttr *` instead?


================
Comment at: clang/include/clang/AST/Type.h:4809
+
+  void Profile(llvm::FoldingSetNodeID &ID) {
+    Profile(ID, WrappedType, Attr);
----------------
You should fix the formatting issue (here and everywhere in the file, I'll stop commenting on them).


================
Comment at: clang/include/clang/AST/TypeLoc.h:904-917
+struct BTFTagAttributedLocInfo {
+  const Attr *TypeAttr;
+};
+
+/// Type source information for an btf_tag attributed type.
+class BTFTagAttributedTypeLoc
+    : public ConcreteTypeLoc<UnqualTypeLoc, BTFTagAttributedTypeLoc,
----------------
This seems a bit suspicious to me -- we are store the same attribute twice (once on the `BTFTagAttributedType` and once on its type loc). Do we need to do that, or can we have the type loc reach through the type pointer to get the attribute from there? (Alternatively, do we want to require callers to have access to an `BTFTagAttributedTypeLoc` in order to query the attribute, similar to how `AttributedType`/`AttributedTypeLoc` work?)


================
Comment at: clang/include/clang/AST/TypeLoc.h:925
+
+  void initializeLocal(ASTContext &Context, SourceLocation loc) {
+    setAttr(nullptr);
----------------
`loc` is unused? (and should have its identifier removed if this is on purpose)


================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:126-127
 
+  /// Write an Attr object.
+  void writeAttr(Attr *attr) { AddAttr(attr); }
+
----------------
This doesn't appear to be used? Or is this called automagically because of the changes in `TypeProperties.td`?


================
Comment at: clang/lib/AST/ASTContext.cpp:4690-4691
 
+QualType ASTContext::getBTFTagAttributedType(BTFTypeTagAttr *attr,
+                                             QualType wrappedType) {
+  llvm::FoldingSetNodeID id;
----------------
Parameter names don't match coding style (same advice as above).


================
Comment at: clang/lib/AST/ASTContext.cpp:4695
+
+  void *insertPos = nullptr;
+  BTFTagAttributedType *type =
----------------
Locals should also make the coding style.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1144
+  SmallVector<llvm::Metadata *, 4> Annots;
+  auto BTFTagAttrTy = dyn_cast<BTFTagAttributedType>(PointeeTy);
+  while (BTFTagAttrTy) {
----------------



================
Comment at: clang/lib/Sema/TreeTransform.h:6872
+  const BTFTagAttributedType *oldType = TL.getTypePtr();
+  StringRef Tag = oldType->getTag();
+  QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
----------------
yonghong-song wrote:
> 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.
> We still need this though, since there are other non-template tree-transforms.

Are we sure any of them can be hit for this new type? It'd be nice to keep this out of the template instantiation bits if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120296