[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:
> 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.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1144
+  SmallVector<llvm::Metadata *, 4> Annots;
+  auto BTFTagAttrTy = dyn_cast<BTFTagAttributedType>(PointeeTy);
+  while (BTFTagAttrTy) {
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > 
> This one may have been missed.
You mean:
```
auto *BTFTagAttrTy = dyn_cast<BTFTagAttributedType>(PointeeTy);
```
I missed it. Will fix in the next revision.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120296