[PATCH] D106614: [Clang] add btf_tag attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Assuming we're back on the same page again, I think the only thing left in this review is a commenting nit.



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6846-6847
+    return;
+  if (hasBTFTagAttr(D, Str))
+    return;
+
----------------
yonghong-song wrote:
> aaron.ballman wrote:
> > This should diagnose that the attribute is being ignored due to the mismatch in tags (and probably have a note to the previous declaration as well so users can see both sides of the conflict).
> Just to make sure we are on the same page. The attribute is ignored because it has been defined in the declaration. This is specifically to handle cases like:
>     #define __tag1 __attribute__((btf_tag("info")))
>     #define __tag2 __attribute__((btf_tag("info2")))
>     int var __tag1 __tag1, __tag2;
> The first __tag1 will be added to declaration successfully, but the second __tag1 will be ignored since there exists an identical one. __tag2 will be added to the declaration successfully.
> 
> I think handleBTFTagAttr() does not handle merging declarations? It is mergeBTFTagAttr below to handle merging? If handleBTFTagAttr() is to handle merging declarations, it will handle attributes the same as below mergeBTFTagAttr, i.e., merging all attributes at the same time doing dedup.
> 
> Do you want issue an warning for ignored attribute?
ohhh, I think we weren't on the same page, sorry about that! I had the semantics wrong in my head -- I was thinking we wanted to reject different tags, but it's the opposite, we want to allow multiple tags so long as they have different arguments. Assuming I have that correct now, this approach looks correct to me (without diagnosing the ignored duplicates).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614