[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.


================
Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
----------------
dblaikie wrote:
> hoy wrote:
> > dblaikie wrote:
> > > hoy wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > Does this need to be down here? Or would the code be a well exercised if it was up next to the go declaration above?
> > > > > > Yes, it needs to be here. Otherwise it will just like the function `bar` above that doesn't get a uniquefied name. I think moving the definition up to right after the declaration hides the declaration.
> > > > > Not sure I follow - do you mean that if the go declaration and go definition were next to each other, this test would (mechanically speaking) not validate what the patch? Or that it would be less legible, but still mechanically correct?
> > > > > 
> > > > > I think it would be (assuming it's still mechanically correct) more legible to put the declaration next to the definition - the comment describes why the declaration is significant/why the definition is weird, and seeing all that together would be clearer to me than spreading it out/having to look further away to see what's going on.
> > > > When the `go` declaration and `go` definition were next to each other, the go function won't get a uniqufied name at all. The declaration will be overwritten by the definition. Only when the declaration is seen by others, such the callsite in `baz`, the declaration makes a difference by having the callsite use a uniqufied name.
> > > > 
> > > > 
> > > Ah! Interesting, good to know. 
> > > 
> > > Is that worth supporting, I wonder? I guess it falls out for free/without significant additional complexity. I worry about the subtlety of the additional declaration changing the behavior here... might be a bit surprising/subtle. But maybe no nice way to avoid it either.
> > It would be ideal if user never writes code like that. Unfortunately it exists with legacy code (such as mysql). I think it's worth supporting it from AutoFDO point of view to avoid a silent mismatch between debug linkage name and real linkage name.
> Oh, I agree that we shouldn't mismatch debug info and the actual symbol name - what I meant was whether code like this should get mangled or not when using unique-internal-linkage names.
> 
> I'm now more curious about this:
> 
> > When the `go` declaration and `go` definition were next to each other, the go function won't get a uniqufied name at all.
> 
> This doesn't seem to happen with the `__attribute__((overloadable))` attribute, for instance - so any idea what's different about uniquification that's working differently than overloadable?
> 
> ```
> $ cat test.c
> __attribute__((overloadable)) static int go(a) int a; {
>   return 3 + a;
> }
> void baz() {
>   go(2);
> }
> $ clang-tot test.c -emit-llvm -S -o - | grep go
>   %call = call i32 @_ZL2goi(i32 2)
> define internal i32 @_ZL2goi(i32 %a) #0 {
> ```
Good question. I'm not sure what's exactly going on but it looks like with the overloadable attribute, the old-style definition is treated as having prototype. But if you do this:

```
__attribute__((overloadable)) 
void baz() {}
```
then there's the error:

```
error: 'overloadable' function 'baz' must have a prototype
void baz() {
```

`void baz() {` does not come with a prototype. That's for sure.  Sounds like `int go(a) int a {;` can have a prototype when it is loadable. I'm wondering why it's not always treated as having prototype, since the parameter type is there.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799