[PATCH] D94154: Unique Internal Linkage Name suffixes must be demangler friendly

Fri Jan 8 15:57:01 PST 2021

hoy added a comment.

In D94154#2488088 <https://reviews.llvm.org/D94154#2488088>, @rnk wrote:

> In D94154#2487891 <https://reviews.llvm.org/D94154#2487891>, @tmsriram wrote:
>
>> In D94154#2487762 <https://reviews.llvm.org/D94154#2487762>, @dblaikie wrote:
>>
>>> @rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).
>>
>> We are going in circles.  For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null.   When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable.  I don't see any issues.
>
> I think David already approved the patch in Phabricator, feel free to land it. David isn't blocking the patch, I think he's suggesting that perhaps we should revisit the IR pass design decision in the near future.
>
> Regarding David's suggestion to only added suffixes to mangled names, wouldn't that be problematic for plain C static functions with no mangling? Suppose you have a program with many C TUs, each of which contains a static free function `foo`. In order for AutoFDO to work, the compiler has to rename all those `foo`'s to something globally unique. That will probably break some debugging functionality, but hey, the user asked for unique names, so they got them.

For static C functions, if we are going to rename them in Clang, can a mangled debug linkage name be assigned to them as if they are decorated with the overloadable attribute?

> Regarding the Microsoft mangling scheme, it is possible that these names will not demangle. Thinking about it more now, in the MS name mangling scheme, clang already tries to generate globally unique names for C++ things with internal linkage (lambdas, anonymous namespaces, etc). It is possible to create entities with non-unique names with unnamed tag types and C static functions, but for the debugger's sake, the mangling scheme makes a best effort to generate globally unique names. This seems like a good reason to actually move this functionality back to the frontend mangler, since only the frontend knows which internal linkage functions already have globally unique IDs embedded in them.
>
> Anyway, I apologize for any confusing or contradictory feedback I may have given. I'm trying to unblock folks, but this doesn't have my full attention.

In D94154#2488088 <https://reviews.llvm.org/D94154#2488088>, @rnk wrote:

> In D94154#2487891 <https://reviews.llvm.org/D94154#2487891>, @tmsriram wrote:
>
>> In D94154#2487762 <https://reviews.llvm.org/D94154#2487762>, @dblaikie wrote:
>>
>>> @rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).
>>
>> We are going in circles.  For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null.   When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable.  I don't see any issues.
>
> I think David already approved the patch in Phabricator, feel free to land it. David isn't blocking the patch, I think he's suggesting that perhaps we should revisit the IR pass design decision in the near future.
>
> Regarding David's suggestion to only added suffixes to mangled names, wouldn't that be problematic for plain C static functions with no mangling? Suppose you have a program with many C TUs, each of which contains a static free function `foo`. In order for AutoFDO to work, the compiler has to rename all those `foo`'s to something globally unique. That will probably break some debugging functionality, but hey, the user asked for unique names, so they got them.
>
> Regarding the Microsoft mangling scheme, it is possible that these names will not demangle. Thinking about it more now, in the MS name mangling scheme, clang already tries to generate globally unique names for C++ things with internal linkage (lambdas, anonymous namespaces, etc). It is possible to create entities with non-unique names with unnamed tag types and C static functions, but for the debugger's sake, the mangling scheme makes a best effort to generate globally unique names. This seems like a good reason to actually move this functionality back to the frontend mangler, since only the frontend knows which internal linkage functions already have globally unique IDs embedded in them.
>
> Anyway, I apologize for any confusing or contradictory feedback I may have given. I'm trying to unblock folks, but this doesn't have my full attention.

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

https://reviews.llvm.org/D94154