[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(); }
----------------
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.



================
Comment at: clang/test/Sema/attr-btf_type_tag.c:29
+int g1 = _Generic((int *)0, int __tag1 *: 0);
+int g2 = _Generic((int __tag1 *)0, int *: 0);
+
----------------
aaron.ballman wrote:
> I'd like a third test case:
> ```
> int g3 = _Generic(0, int __tag1 * : 0, int * : 0, default : 0); // expected-error {{something about duplicate compatible associations}}
> ```
Will add. I will break the above test into multiple lines. Otherwise, there will be multiple errors in the same line.


================
Comment at: clang/test/Sema/attr-btf_type_tag.c:31-37
+// The btf_type_tag attribute will be ignored during overloadable type matching
+void __attribute__((overloadable)) bar1(int __tag1 *a);
+void __attribute__((overloadable)) bar2(int *a);
+void foo2(int *a, int __tag1 *b) {
+  bar1(a);
+  bar2(b);
+}
----------------
aaron.ballman wrote:
> yonghong-song wrote:
> > aaron.ballman wrote:
> > > This test looks confused -- those functions have different names, so they're not overloading one another. Even if they were named with the same identifier, it wouldn't be sufficient to test which gets called (you can redeclare overloads as in https://godbolt.org/z/aebMbY1aM).
> > > 
> > > I think this case needs a codegen test to see which function is actually dispatched to.
> > sorry for confusion. the test is bad. For the example in the above godbolt.org link, we have
> > ```
> > __attribute__((overloadable)) void func(int a);
> > __attribute__((overloadable)) void func(int a);
> > ```
> > Such multiple declarations are actually okay since we allow duplicated declarations. As you mentioned, we really want to know which function definition it picks with overloadable attribute.
> > I tried a few examples:
> > ```
> > $ cat test.c
> > #if 1
> > #define __attr __attribute__((btf_type_tag("tag1")))
> > #else
> > #define __attr __attribute__((noderef))
> > #endif
> > 
> > void bar1(int __attr *);
> > void bar2(int *);
> > void isdigit1(int __attr *a) __attribute__((overloadable)) {
> >   bar1(a);
> > }
> > void isdigit1(int *a) __attribute__((overloadable)) {
> >   bar2(a);
> > }
> > 
> > void foo(int __attr *a, int *b) {
> >   isdigit1(a);
> >   isdigit1(b);
> > }
> > 
> > $ clang -g -std=c2x test.c -S -O2 -emit-llvm -DUSE_TAG
> > test.c:12:6: error: redefinition of 'isdigit1'
> > void isdigit1(int *a) __attribute__((overloadable)) {
> >      ^
> > test.c:9:6: note: previous definition is here
> > void isdigit1(int __attr *a) __attribute__((overloadable)) {
> >      ^
> > 1 error generated.
> > $ clang -g -std=c2x test.c -S -O2 -emit-llvm
> > test.c:12:6: error: redefinition of 'isdigit1'
> > void isdigit1(int *a) __attribute__((overloadable)) {
> >      ^
> > test.c:9:6: note: previous definition is here
> > void isdigit1(int __attr *a) __attribute__((overloadable)) {
> >      ^
> > 1 error generated.
> > $
> > ```
> > 
> > so just attribute itself cannot differentiate overloadable functions. In the above test.c, if I changed one of isdigle1 and corresponding bar?() function from 'int' to 'float', compilation can succeed.
> > 
> > So I will add overloadable failure and success case in Sema/ directory. I am not sure whether a codegen case is needed for the case like
> > ```
> > void bar1(float __attr *);
> > void bar2(int *);
> > void isdigit1(float __attr *a) __attribute__((overloadable)) {
> >   bar1(a);
> > }
> > void isdigit1(int *a) __attribute__((overloadable)) {
> >   bar2(a);
> > }
> > ```
> > or not. But if you think it is necessary, I am happy to add it.
> Okay, I am fine with that behavior. CodeGen tests are only necessary if the overloads can actually be defined -- since they can't, I think the Sema tests are sufficient to show that these are redeclarations and not independent overloads.
Thanks for confirmation!


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

https://reviews.llvm.org/D120296