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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D98799#2860881 , @dblaikie wrote:

> In D98799#2850700 , @dblaikie wrote:
>
>> In D98799#2850682 , @ahatanak wrote:
>>
>>> I think I've fixed all the places in CodeGen that create fake 
>>> `FunctionDecl`s and would cause clang to crash.
>>
>> Thanks, I really appreciate it! I'll have a go at unifying this mangled V 
>> unique internal linkage name stuff soon.
>
> Committed a patch to resolve the inconsistencies with K declarations, 
> `__attribute__((overloadable))` (which would mangle overloadable K 
> declarations, but would not attach the mangled name to the debug info 
> metadata), and `-funique-internal-linkage-names` (which wouldn't 
> mangle/suffix K declarations (but would make this choice consistently 
> between debug info and the actual symbol name) - now all those cases can/do 
> mangle the names: 6c9559b67b91966bfeff9e17808a3e84a92e64a0 
> 

Thanks for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D98799#2850700 , @dblaikie wrote:

> In D98799#2850682 , @ahatanak wrote:
>
>> I think I've fixed all the places in CodeGen that create fake 
>> `FunctionDecl`s and would cause clang to crash.
>
> Thanks, I really appreciate it! I'll have a go at unifying this mangled V 
> unique internal linkage name stuff soon.

Committed a patch to resolve the inconsistencies with K declarations, 
`__attribute__((overloadable))` (which would mangle overloadable K 
declarations, but would not attach the mangled name to the debug info 
metadata), and `-funique-internal-linkage-names` (which wouldn't mangle/suffix 
K declarations (but would make this choice consistently between debug info 
and the actual symbol name) - now all those cases can/do mangle the names: 
6c9559b67b91966bfeff9e17808a3e84a92e64a0 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D98799#2850682 , @ahatanak wrote:

> I think I've fixed all the places in CodeGen that create fake `FunctionDecl`s 
> and would cause clang to crash.

Thanks, I really appreciate it! I'll have a go at unifying this mangled V 
unique internal linkage name stuff soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.
Herald added a subscriber: modimo.

I think I've fixed all the places in CodeGen that create fake `FunctionDecl`s 
and would cause clang to crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I think these places have to be fixed too:
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGBuiltin.cpp#L1705
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGNonTrivialStruct.cpp#L476
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGObjC.cpp#L3692
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGObjC.cpp#L3776
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2618

This one is okay since it's creating a function that doesn't take any 
parameters:
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGStmtOpenMP.cpp#L449


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D98799#2820198 , @ahatanak wrote:

> There are several files other than CGBlocks.cpp in which 
> `FunctionDecl::Create` is called to create fake FunctionDecls. Do those 
> places have to be fixed too?

This issue in this particular case was that the function that was created never 
had its parameter info properly initialized (so that this call succeeds: 
https://github.com/llvm-mirror/clang/blob/aa231e4be75ac4759c236b755c57876f76e3cf05/lib/AST/ItaniumMangle.cpp#L2908
 ). Any examples we could sample/look at?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

There are several files other than CGBlocks.cpp in which `FunctionDecl::Create` 
is called to create fake FunctionDecls. Do those places have to be fixed too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Here is the patch that removes the fake FunctionDecls: 
https://reviews.llvm.org/D104082


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, Akira.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D98799#2794546 , @ahatanak wrote:

> Yes, I think I can fix that.

Thanks a lot! If you could follow-up here when that's committed, I can 
follow-up with a fix/improvement for the debug info mangling issue we've been 
discussing here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Yes, I think I can fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D98799#2794366 , @ahatanak wrote:

> I don't know know why these fake FunctionDecls are needed, but it looks like 
> it's okay to avoid creating them. I see a few debug info tests failing if 
> `GlobalDecl()` instead of a fake function is passed to `StartFunction`, but 
> it looks like the test check strings should be changed.
>
> AFAICT `CodeGenFunction::GenerateCopyHelperFunction` has been creating these 
> fake FunctionDecls from the beginning and subsequent patches didn't fix it:
>
> https://github.com/llvm/llvm-project/commit/0c74327715af823930cb583490d315f64ef8de4e

Would you be able to make that change, if it's not too much work? (I'm not as 
familiar with the Objective C(++) stuff)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I don't know know why these fake FunctionDecls are needed, but it looks like 
it's okay to avoid creating them. I see a few debug info tests failing if 
`GlobalDecl()` instead of a fake function is passed to `StartFunction`, but it 
looks like the test check strings should be changed.

AFAICT `CodeGenFunction::GenerateCopyHelperFunction` has been creating these 
fake FunctionDecls from the beginning and subsequent patches didn't fix it:

https://github.com/llvm/llvm-project/commit/0c74327715af823930cb583490d315f64ef8de4e


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: ahatanak.
rjmccall added a comment.

Hmm.  CC'ing Akira.  Akira, do you know why we're building a fake FunctionDecl 
as part of emitting these helper functions?  Is it something we can avoid?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for taking a look, @rjmccall - I really appreciate it!

Sorry I'm doing a bad job describing (admittedly I'm pretty confused about all 
this, which is most of the issue) the issue - I'll try to clarify as best I can.

In D98799#2781200 , @rjmccall wrote:

> In D98799#2761684 , @dblaikie wrote:
>
>> OK - poked around a bit more to better understand this, so attempting to 
>> summarize my current understanding (excuse the repetition from previous 
>> parts of this thread) and results.
>>
>> - `__attribute__((overloadable))` can/does mangle K C style declarations 
>> 
>> - with this patch (currently committed), `-funique-internal-linkage-names` 
>> does not mangle K C style declarations (see `bar` in the included test 
>> case, unmangled)
>> - I'd like to avoid that divergence if possible
>> - Changing the debug info code to be more generous with names it mangles (by 
>> using `FD->getType()->getAs()` rather than 
>> `hasPrototype()`) causes problems
>>   - Specifically: Objective C blocks (which have a `FunctionProtoType` type, 
>> but `!hasPrototype` it seems) are missing parameter info so this 
>> 
>>  call crashes
>> - There doesn't seem to be any way to test for this property of the 
>> `FunctionDecl` that I can see - where it has a type, but doesn't have 
>> parameter info
>>
>> Trying to pull in some folks who might know what's going on here/be able to 
>> suggest a way to split these cases if needed, or fix the block 
>> `FunctionDecl`s to have param info. @rjmccall @JDevlieghere - I'd really 
>> appreciate some help here.
>
> Unlike lambdas, `BlockDecl` is not a subclass of `FunctionDecl`, so I'm not 
> quite sure what you're asking — there shouldn't be a way for that line to 
> crash on a `BlockDecl` because `FD` should be null.
>
> Not sure what you mean by block `FunctionDecl` — a block doesn't make a 
> `FunctionDecl`.  If it's useful for `BlockDecl` to return

Did this ^ get cut off in some way ("If it's useful for `BlockDecl` to return" 
... something?).

Anyway - so this crash only seems to happen in Objective C++ tests 
. It's not the block itself, but I'm 
guessing some implementation function, specifically:

  FunctionDecl 0xf921248 <>  
__Block_byref_object_copy_ 'void (void *, void *)' static <<>>

This `FunctionDecl`'s type is a `FunctionProtoType`, but doesn't have any 
parameter type info, which seems weird. Maybe that's the bug - perhaps whatever 
creates this should be adding type info like for other `FunctionProtoType`d 
`FunctionDecl`s?

Ah, here it is: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBlocks.cpp#L1958
 - that creates this seemingly weird FunctionDecl that has a prototype type, 
but if you try to query the type of its parameters, it'll crash.

Other `FunctionDecl`s get their `ParmVarDecl`s in places like this: 
https://github.com/llvm/llvm-project/blob/7daa18215905c831e130c7542f17619e9d936dfc/clang/lib/Sema/SemaDecl.cpp#L9478
 and look like this:

  FunctionDecl 0xf923bb0  col:42 go 'int (int)' static
  `-ParmVarDecl 0xf923ae8  col:49 a 'int'

Should the synthetic `FunctionDecl` created by CGBlocks.cpp be changed to be 
more fully featured and have valid `ParmVarDecl`s?

[aside: Looks like the bug here that I'm interested in sort of already exists 
apart from the unique linkage name case here - we don't add the mangled name of 
`__attribute__((overloadable))` to DWARF even though we should, and if I try to 
fix that the obvious way (testing `FD->getType()->getAs()` 
instead of `FD->hasPrototype()` in `collectFunctionDeclProps` then it crashes 
the ObjC++ block tests for the above reason]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D98799#2761684 , @dblaikie wrote:

> OK - poked around a bit more to better understand this, so attempting to 
> summarize my current understanding (excuse the repetition from previous parts 
> of this thread) and results.
>
> - `__attribute__((overloadable))` can/does mangle K C style declarations 
> 
> - with this patch (currently committed), `-funique-internal-linkage-names` 
> does not mangle K C style declarations (see `bar` in the included test 
> case, unmangled)
> - I'd like to avoid that divergence if possible
> - Changing the debug info code to be more generous with names it mangles (by 
> using `FD->getType()->getAs()` rather than 
> `hasPrototype()`) causes problems
>   - Specifically: Objective C blocks (which have a `FunctionProtoType` type, 
> but `!hasPrototype` it seems) are missing parameter info so this 
> 
>  call crashes
> - There doesn't seem to be any way to test for this property of the 
> `FunctionDecl` that I can see - where it has a type, but doesn't have 
> parameter info
>
> Trying to pull in some folks who might know what's going on here/be able to 
> suggest a way to split these cases if needed, or fix the block 
> `FunctionDecl`s to have param info. @rjmccall @JDevlieghere - I'd really 
> appreciate some help here.

Unlike lambdas, `BlockDecl` is not a subclass of `FunctionDecl`, so I'm not 
quite sure what you're asking — there shouldn't be a way for that line to crash 
on a `BlockDecl` because `FD` should be null.

Not sure what you mean by block `FunctionDecl` — a block doesn't make a 
`FunctionDecl`.  If it's useful for `BlockDecl` to return


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: JDevlieghere, rjmccall.
dblaikie added a comment.

OK - poked around a bit more to better understand this, so attempting to 
summarize my current understanding (excuse the repetition from previous parts 
of this thread) and results.

- `__attribute__((overloadable))` can/does mangle K C style declarations 

- with this patch (currently committed), `-funique-internal-linkage-names` does 
not mangle K C style declarations (see `bar` in the included test case, 
unmangled)
- I'd like to avoid that divergence if possible
- Changing the debug info code to be more generous with names it mangles (by 
using `FD->getType()->getAs()` rather than `hasPrototype()`) 
causes problems
  - Specifically: Objective C blocks (which have a `FunctionProtoType` type, 
but `!hasPrototype` it seems) are missing parameter info so this 

 call crashes
- There doesn't seem to be any way to test for this property of the 
`FunctionDecl` that I can see - where it has a type, but doesn't have parameter 
info

Trying to pull in some folks who might know what's going on here/be able to 
suggest a way to split these cases if needed, or fix the block `FunctionDecl`s 
to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help 
here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D98799#2757959 , @dblaikie wrote:

>> I took another look. I think the divergence comes from 
>> getAs vs hasPrototype. The debug data generation uses 
>> hasPrototype while getAs is used as overloadable 
>> attribute processing as long as unique linkage name processing before this 
>> change. More specifically, the following function definition is represented 
>> by FunctionProtoType while it does not hasPrototype.
>
> Ah, sorry, maybe I'm coming around to this - so you're saying that the test 
> in `ItaniumMangleContextImpl::isUniqueInternalLinkageDecl` must match the 
> check in `CGDebugInfo::collectFunctionDeclProps` And when they diverge 
> something bad happens? (could you refresh me on what that breaks - something 
> crashes in ObjectiveC test cases? Or the tests fail?)
>
> I wonder whether we should change both of them then?

Yes, from AutoFDO point of view they should match, since both ELF symbol names 
and debug names are used to generate profiles. The current patch unifies them 
but it causes divergence from overloadable. Previously, 
`ItaniumMangleContextImpl::isUniqueInternalLinkageDecl`  was consistent with 
overloadable. I tried to unify in the other way so that all the three places 
are consistent, i.e, using `FD->getType()->getAs()` 
everywhere and it caused the following tests to fail:

  Clang :: CodeGenCXX/cp-blocks-linetables.cpp
  Clang :: CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
  Clang :: CodeGenCXX/debug-info-blocks.cpp
  Clang :: CodeGenObjCXX/nested-ehlocation.mm
  Clang :: CodeGenObjCXX/property-objects.mm
  Clang :: CodeGenObjCXX/synthesized-property-cleanup.mm 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> I took another look. I think the divergence comes from 
> getAs vs hasPrototype. The debug data generation uses 
> hasPrototype while getAs is used as overloadable attribute 
> processing as long as unique linkage name processing before this change. More 
> specifically, the following function definition is represented by 
> FunctionProtoType while it does not hasPrototype.

Ah, sorry, maybe I'm coming around to this - so you're saying that the test in 
`ItaniumMangleContextImpl::isUniqueInternalLinkageDecl` must match the check in 
`CGDebugInfo::collectFunctionDeclProps` And when they diverge something bad 
happens? (could you refresh me on what that breaks - something crashes in 
ObjectiveC test cases? Or the tests fail?)

I wonder whether we should change both of them then?




Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
+}
+
+

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

[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;
+}
+
+

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
+}
+
+

hoy wrote:
> aaron.ballman wrote:
> > dblaikie wrote:
> > > hoy wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > > > > > > > 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? 
> > > > > > > > > > > > > > > > 

[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;
+}
+
+

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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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

[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:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > aaron.ballman wrote:
> > > > > > dblaikie wrote:
> > > > > > > hoy wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > hoy wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > > > 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 - 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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

[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:
> aaron.ballman wrote:
> > dblaikie wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > hoy wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > 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 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

aaron.ballman wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > hoy wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > hoy wrote:
> > > > > > > > > > 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);
> > > > > > > > > > > }
> > > > > > > > > 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > 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 {
> > > > > > > > > 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: aaron.ballman.
dblaikie 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;
+}
+
+

hoy wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > hoy wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > hoy wrote:
> > > > > > > > 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 
> > > > > > > > 

[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:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > 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

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

[David Blaikie via Phabricator via cfe-commits]

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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

hoy wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > 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.
> > > > > 
> > > > > 
> > > > > 
> > > > Yeah, that seems like that divergence be worth understanding (& if 
> > 

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a subscriber: bruno.
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:
> > > > > > > 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.
> > > > 
> > > > 
> > > > 
> > > Yeah, that seems like that divergence be worth understanding (& if 
> > > possible fixing/avoiding/merging). Ensuring these features don't have 
> > > subtle divergence I think will be valuable to having a model that's easy 
> > > to explain/understand/modify/etc.
> > I took 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: aprantl, rsmith.
dblaikie 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;
+}
+
+

hoy wrote:
> dblaikie wrote:
> > hoy wrote:
> > > 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.
> > > 
> > > 
> > > 
> > Yeah, that seems like that divergence be worth understanding (& if possible 
> > fixing/avoiding/merging). Ensuring these features don't have subtle 
> > divergence I think will be valuable to having a model that's easy to 
> > explain/understand/modify/etc.
> I took another look. I think the divergence comes from 
> `getAs` vs `hasPrototype`. The debug data generation uses 
> `hasPrototype` while `getAs` is used as overloadable 
> attribute processing as long as unique linkage name 

[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:
> > > > > > > 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.
> > 
> > 
> > 
> Yeah, that seems like that divergence be worth understanding (& if possible 
> fixing/avoiding/merging). Ensuring these features don't have subtle 
> divergence I think will be valuable to having a model that's easy to 
> explain/understand/modify/etc.
I took another look. I think the divergence comes from 
`getAs` vs `hasPrototype`. The debug data generation uses 
`hasPrototype` while `getAs` is used as overloadable 
attribute processing as long as unique linkage name processing before this 
change. More specifically, the following function definition is represented by 
`FunctionProtoType`  while it does not `hasPrototype`.

```
static int go(a) int a; {
  return 3 + a;
}
```

I was trying to have `CGDebugInfo` to check 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

hoy wrote:
> 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.
> 
> 
> 
Yeah, that seems like that divergence be worth understanding (& if possible 
fixing/avoiding/merging). Ensuring these features don't have subtle divergence 
I think will be valuable to having a model that's easy to 
explain/understand/modify/etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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 {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
+}
+
+

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hongtao Yu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc1812a0ad75: [UniqueLinkageName] Use consistent checks when 
mangling symbo linkage name and… (authored by hoy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

Files:
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.c


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited 
-dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | 
FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under 
-funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, 
even 
+// if its definition doesn't come with a valid prototype, but the declaration 
here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]()
+// UNIQUE: define internal i32 @bar(i32 %a)
+// UNIQUE: define internal i32 @_ZL2goi.[[MODHASH]](i32 %a)
 // UNIQUE: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // UNIQUE: distinct !DISubprogram(name: "foo", linkageName: 
"_ZL3foov.[[MODHASH]]"{{.*}})
+// UNIQUE: distinct !DISubprogram(name: "go", linkageName: 
"_ZL2goi.[[MODHASH]]"{{.*}})
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3522,7 +3522,7 @@
llvm::DIScope *,
llvm::DINodeArray ,
llvm::DINode::DIFlags ) {
-  const auto *FD = cast(GD.getDecl());
+  const auto *FD = cast(GD.getCanonicalDecl().getDecl());
   Name = getFunctionName(FD);
   // Use mangled name as linkage name for C/C++ functions.
   if (FD->hasPrototype()) {
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -640,7 +640,7 @@
 
   // For C functions without prototypes, return false as their
   // names should not be mangled.
-  if (!FD->getType()->getAs())
+  if (!FD->hasPrototype())
 return false;
 
   if (isInternalLinkageDecl(ND))


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under -funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, even 
+// if its definition doesn't come with a valid prototype, but the declaration here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define 

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 331660.
hoy added a comment.

Addresssing David's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

Files:
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.c


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited 
-dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | 
FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under 
-funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, 
even 
+// if its definition doesn't come with a valid prototype, but the declaration 
here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]()
+// UNIQUE: define internal i32 @bar(i32 %a)
+// UNIQUE: define internal i32 @_ZL2goi.[[MODHASH]](i32 %a)
 // UNIQUE: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // UNIQUE: distinct !DISubprogram(name: "foo", linkageName: 
"_ZL3foov.[[MODHASH]]"{{.*}})
+// UNIQUE: distinct !DISubprogram(name: "go", linkageName: 
"_ZL2goi.[[MODHASH]]"{{.*}})
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3522,7 +3522,7 @@
llvm::DIScope *,
llvm::DINodeArray ,
llvm::DINode::DIFlags ) {
-  const auto *FD = cast(GD.getDecl());
+  const auto *FD = cast(GD.getCanonicalDecl().getDecl());
   Name = getFunctionName(FD);
   // Use mangled name as linkage name for C/C++ functions.
   if (FD->hasPrototype()) {
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -640,7 +640,7 @@
 
   // For C functions without prototypes, return false as their
   // names should not be mangled.
-  if (!FD->getType()->getAs())
+  if (!FD->hasPrototype())
 return false;
 
   if (isInternalLinkageDecl(ND))


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under -funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, even 
+// if its definition doesn't come with a valid prototype, but the declaration here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]()
+// UNIQUE: define internal i32 @bar(i32 %a)
+// UNIQUE: define 

[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/lib/CodeGen/CGDebugInfo.cpp:3525-3526
llvm::DINode::DIFlags ) {
-  const auto *FD = cast(GD.getDecl());
+  GlobalDecl CanonicalGD = GD.getCanonicalDecl();
+  const auto *FD = cast(CanonicalGD.getDecl());
   Name = getFunctionName(FD);

dblaikie wrote:
> I'd probably roll this into the expression rather than adding another named 
> variable - it doesn't seem to add much readability to me at least. Up to you.
Good point, fixed.



Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:18
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{

tmsriram wrote:
> Nice test, I didnt know you could do this!
I didn't know either. It's an old C usage. We hit this when compiling mysql.



Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:18
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{

Nice test, I didnt know you could do this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - couple of minor/optional things.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3525-3526
llvm::DINode::DIFlags ) {
-  const auto *FD = cast(GD.getDecl());
+  GlobalDecl CanonicalGD = GD.getCanonicalDecl();
+  const auto *FD = cast(CanonicalGD.getDecl());
   Name = getFunctionName(FD);

I'd probably roll this into the expression rather than adding another named 
variable - it doesn't seem to add much readability to me at least. Up to you.



Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy created this revision.
Herald added a subscriber: wenlei.
hoy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

C functions may be declared and defined in different prototypes like below. 
This patch unifies the checks for mangling names in symbol linkage name 
emission and debug linkage name emission so that the two names are consistent.

static int go(int);

static int go(a) int a;
{

  return a;

}

Test Plan:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98799

Files:
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.c


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited 
-dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | 
FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under 
-funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, 
even 
+// if its definition doesn't come with a valid prototype, but the declaration 
here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]()
+// UNIQUE: define internal i32 @bar(i32 %a)
+// UNIQUE: define internal i32 @_ZL2goi.[[MODHASH]](i32 %a)
 // UNIQUE: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // UNIQUE: distinct !DISubprogram(name: "foo", linkageName: 
"_ZL3foov.[[MODHASH]]"{{.*}})
+// UNIQUE: distinct !DISubprogram(name: "go", linkageName: 
"_ZL2goi.[[MODHASH]]"{{.*}})
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3522,7 +3522,8 @@
llvm::DIScope *,
llvm::DINodeArray ,
llvm::DINode::DIFlags ) {
-  const auto *FD = cast(GD.getDecl());
+  GlobalDecl CanonicalGD = GD.getCanonicalDecl();
+  const auto *FD = cast(CanonicalGD.getDecl());
   Name = getFunctionName(FD);
   // Use mangled name as linkage name for C/C++ functions.
   if (FD->hasPrototype()) {
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -640,7 +640,7 @@
 
   // For C functions without prototypes, return false as their
   // names should not be mangled.
-  if (!FD->getType()->getAs())
+  if (!FD->hasPrototype())
 return false;
 
   if (isInternalLinkageDecl(ND))


Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -8,21 +8,48 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under -funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, even 
+// if its definition doesn't come with a valid prototype, but the declaration here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 //