[PATCH] D135710: [lto] Do not try to internalize symbols with escaped name

Wed Oct 12 08:13:11 PDT 2022

tejohnson added a comment.

Ok thanks all for the context. I spent some time poking around this morning and I think this approach is fine for now but have a couple of questions and comments.

First, I'm wondering why this isn't an issue for regular LTO which by default does index based liveness. Specifically, see the following which is for both regular and thin LTO:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTO.cpp#L1006-L1032

Since it sounds like we get separate combined index entries for the 2 names, I'm wondering why one of them (the one that doesn't match the global resolution IRName) isn't being incorrectly marked dead by that analysis and then subsequently removed by regular LTO here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTO.cpp#L861-L873

Second, I see that the big problem we have is that we compute the GUID during ThinLTO in particular using the static version of getGlobalIdentifier, since we don't have IR, and thus it isn't easy to modify that to take the mangling into account. But it looks like we would just need the triple, which is available in the InputFile object created for all bitcode files during linking. That could be used here in the linker interface to LTO and then perhaps stored in the index alongside the corresponding module path for later use during thinlto. It looks like there is handling in llvm/lib/IR/Mangler.cpp that does this mangling that could be modified to then work off that information instead of accessing it from the GV. If this is correct I wonder if we should document this idea somewhere for future work. It is fairly invasive though so I'm fine with the workaround here for now.

Couple more comments below.

In D135710#3850614 <https://reviews.llvm.org/D135710#3850614>, @int3 wrote:

> My bad, I had a proper look and yeah this seems to do the right thing.
>
> I'm wondering if the test should use `llvm-lto` instead of `ld64.lld` (and placed under `llvm/test/LTO`) since this is technically not an LLD-specific issue. I don't feel strongly about it though.

Yeah, I think llvm-lto2 (not llvm-lto, which tests the legacy LTO interface) would be a better tool to test this with, since it doesn't seem lld-specific. Either under llvm/test/ThinLTO, or if this is also an issue with regular LTO, then under llvm/test/LTO.

> If we are keeping the test under `lld/`, I would prefer we combined both test files into one (relying more on `split-file`).

Looks like the tests already use split-file? Maybe that was changed after this comment though.

================
Comment at: lld/test/MachO/hidden-escaped-symbols-alt.ll:5
+; RUN: opt -module-summary %t/ref.ll -o %t/ref.o
+; RUN: %lld %t/hide-me.o %t/ref.o -o %t/out
+
----------------
I know this is just checking that it links correctly without error, but it would be better to add an explicit CHECK for the linked symbol too. Also please add a comment at the top about what is being tested. Ditto for other new test.

================
Comment at: llvm/lib/LTO/LTO.cpp:571
+    // In rare occasion, the symbol used to initialize GlobalRes has a different
+    // IRName from the inspected Symbol. In that case, we have the same actual
+    // Symbol that can get two different GUID, leading to some invalid
----------------
Can you put in more details on why (with the MachO mangling as an example).

================
Comment at: llvm/lib/LTO/LTO.cpp:573
+    // Symbol that can get two different GUID, leading to some invalid
+    // dismissal. Workaround this by marking the GlobalRes external.
+    if (GlobalRes.IRName != Sym.getIRName()) {
----------------
I assume by "dismissal" you mean "internalization" or "dead code elimination". Can you change this to mention which of those is happening incorrectly, since "dismissal" isn't really a term used elsewhere in LTO afaik.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135710