[PATCH] D84803: [ThinLTO][MachO] Preserve both possible GUIDs from exported list

Thu Jul 30 10:06:36 PDT 2020

steven_wu added a comment.

In D84803#2185095 <https://reviews.llvm.org/D84803#2185095>, @tejohnson wrote:

> In D84803#2184985 <https://reviews.llvm.org/D84803#2184985>, @steven_wu wrote:
>
>> In D84803#2184976 <https://reviews.llvm.org/D84803#2184976>, @tejohnson wrote:
>>
>>> Not very familiar with MachO. Can you clarify the issue - is it the case that some modules will use the mangled names and some won't? Otherwise it seems like it could use either all mangled or all unmangled.
>>
>> The issue is that for macho, for a symbol in the object file (symbol table seen by the linker), let's say "_symbol". It can be either. "symbol" or "\01_symbol" in the bitcode file. When user/linker says it want to preserve "_symbol", you don't know what the GUID for that symbol is because it can be both.
>
> I see - so this is an issue with the way the user has specified the name through some kind of option to say it needs to be preserved.

The deeper issue is that thinLTO doesn't know "symbol" and "\01_symbol" is the same symbol so it will not optimize them but neither will fullLTO I guess.

>> Now I think about the patch, it is also not correct that it will preserve "__symbol". Is there any reason why GUID is calculated based on `getGlobalIdentifier()`? `getGlobalIdentifier()` drops the `\01` prefix so "\01_symbol" is the same GUID is "_symbol".
>
> It is computed based on getGlobalIdentifier because we need to make sure local names are globalized, and we by design want to ensure the global identifier scheme is the same as that used by PGO (for indirect call promotion). Looks like the code to strip out the '\01' has been there from before ThinLTO (for the PGO handling).

I don't know what is the best way to deal with this because for macho, "_symbol" and "\01_symbol" are two different symbols. Maybe drop `\01_` for macho is a better option.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84803