[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/TypeLoc.h:923
+  void initializeLocal(ASTContext &Context, SourceLocation loc) {}
+
+  QualType getInnerType() const { return getTypePtr()->getWrappedType(); }
----------------
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".


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

https://reviews.llvm.org/D120296