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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think we're pretty close; it looks like there's still one unresolved conversation about template instantiation needs and whether we can remove that code or not.



================
Comment at: clang/include/clang/AST/TypeLoc.h:923
+  void initializeLocal(ASTContext &Context, SourceLocation loc) {}
+
+  QualType getInnerType() const { return getTypePtr()->getWrappedType(); }
----------------
yonghong-song wrote:
> yonghong-song wrote:
> > aaron.ballman wrote:
> > > yonghong-song wrote:
> > > > aaron.ballman wrote:
> > > > > Do we also need something like this? https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TypeLoc.h#L1187
> > > > I didn't see getLocalDataSize() in AttributedTypeLoc so it is not needed for AttributedTypeLoc. BTFTagAttributedTypeLoc usage is very similar to AttributedTypeLoc, so I think we are fine here.
> > > The main difference is that `AttributedLocInfo` has a member and `BTFTagAttributedLocInfo` is empty. This is why I think it's closer to an `AdjustedLocInfo` object which also is an empty struct.
> > I think adding getLocalDataSize() return 0 should be OK. But it looks like we don't need to do it, at least for BTFTagAttributedTypeLoc. The following are some details.
> > 
> > The parent class of BTFTagAttributedTypeLoc is ConcreteTypeLoc, which contains:
> > 
> >   unsigned getLocalDataSize() const {
> >     unsigned size = sizeof(LocalData);
> >     unsigned extraAlign = asDerived()->getExtraLocalDataAlignment();
> >     size = llvm::alignTo(size, extraAlign);
> >     size += asDerived()->getExtraLocalDataSize();
> >     return size;
> >   }
> > 
> >   unsigned getExtraLocalDataSize() const {
> >     return 0;
> >   }
> > 
> >   unsigned getExtraLocalDataAlignment() const {
> >     return 1;
> >   } 
> > 
> > Manually calculating getLocalDataSize() can get the same return value 0. So I think it is okay not to define  getLocalDataSize in BTFTagAttributedTypeLoc. 
> > 
> > The AdjustedLocInfo seems having some inconsistency, at least based on comments:
> > 
> > struct AdjustedLocInfo {}; // Nothing.
> >   unsigned getLocalDataSize() const {
> >     // sizeof(AdjustedLocInfo) is 1, but we don't need its address to be unique
> >     // anyway.  TypeLocBuilder can't handle data sizes of 1.
> >     return 0;  // No data.
> >   }
> > Maybe previously AdjustedLocInfo size is 1 and the implementation of getLocalDataSize() to set size to 0 explicitly in which case, the function is needed inside AdjustedTypeLoc. It might be needed now since AdjustedLocInfo size is 0.
> > 
> For
> > Maybe previously AdjustedLocInfo size is 1 and the implementation of getLocalDataSize() to set size to 0 explicitly in which case, the function is needed inside AdjustedTypeLoc. It might be needed now since AdjustedLocInfo size is 0.
> I mean "it might not be needed now since AdjustedLocInfo size is 0".
Ahh, I see why we get away with it then. Thank you!


================
Comment at: clang/lib/Sema/TreeTransform.h:6872
+  const BTFTagAttributedType *oldType = TL.getTypePtr();
+  StringRef Tag = oldType->getTag();
+  QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
----------------
aaron.ballman wrote:
> 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.
I think this may be the only unresolved conversation in the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120296