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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> > 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.


================
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);
+
----------------
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}}
```


================
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);
+}
----------------
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.


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

https://reviews.llvm.org/D120296