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

Thu Jul 30 09:24:24 PDT 2020

tejohnson added a comment.

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.

> 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).

================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:280
+    // symbols "_$NAME", we need to preserve the corresponding GUID for "$NAME"
+    // and "\01_$NAME".
     if (TheTriple.isOSBinFormatMachO() && Name.size() > 0 && Name[0] == '_')
----------------
The code doesn't seem to match the new comment. Should it be also preserving "\01_$NAME"?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84803