LTO: call getRealLinkageName on IRNames before feeding to getGUID

[inglorion]

Summary

GlobalValue has two getGUID methods: an instance method and a static method. The static method takes a string, which is expected to be what GlobalValue::getRealLinkageName() would return. In LTO.cpp, we were not doing this consistently, sometimes passing an IR name instead. This change makes it so that we call getRealLinkageName() first, making the static getGUID return value consistent with the instance method. Without this change, compiling FileCheck with ThinLTO on Windows fails with numerous undefined symbol errors. With the change, it builds successfully.

[inglorion]

Thanks, pcc, for helping debug this, and rnk for the suggestion to use getRealLinkageName to solve it.

[mehdi_amini]

Can we get consistently GlobalValue::getGUID taking an IRName and taking care of calling getRealLinkageName? (assuming it is what's expected)

[inglorion]

@mehdi_amini, we have other places where we call GlobalValue::getGUID with a name that is not an IR name, e.g. in BitcodeReader.cpp. I wouldn't be opposed to moving to a design where it's harder to pass the wrong kind of name, or even a design where the IR name and the RealLinkageName and the GlobalIdentifier aren't all different things, but that is going to be a larger change. Can we get this fix in in the meantime?

[mehdi_amini]

In D31444#712721, @inglorion wrote:

@mehdi_amini, we have other places where we call GlobalValue::getGUID with a name that is not an IR name, e.g. in BitcodeReader.cpp. I wouldn't be opposed to moving to a design where it's harder to pass the wrong kind of name, or even a design where the IR name and the RealLinkageName and the GlobalIdentifier aren't all different things, but that is going to be a larger change. Can we get this fix in in the meantime?

Sure, it would be a separate patch (NFC) than this bugfix, I didn't mean to block this.
I don't know about windows and getRealLinkageName enough to know if this is correct and if it is the only places we need to do this, so I rather have @pcc approving this patch.

Also I'm wondering about PGO, which is also using GUIDs (maybe not impacted since the linker does not need to be involved)...

[pcc]

This seems reasonable for now, with a test case.

In D31444#712714, @mehdi_amini wrote:

Can we get consistently GlobalValue::getGUID taking an IRName and taking care of calling getRealLinkageName? (assuming it is what's expected)

Something like that seems reasonable to me as a followup.
Here are all the places where we are passing a string to getGUID: http://llvm-cs.pcc.me.uk/include/llvm/IR/GlobalValue.h/rgetGUID
Some are wrong (passing a value name) and some are passing the result of getGlobalIdentifier.

So maybe we should have just one static function:

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That should hopefully make the API harder to misuse.

(Although that would leave us with a somewhat awkward API for type identifier GUIDs, so maybe we should move the GUID generation out of GlobalValue as I think we discussed at one point.)

lib/LTO/LTO.cpp

1016 ↗

(On Diff #93318)

80 cols

1035 ↗

(On Diff #93318)

80 cols

[rnk]

In D31444#712724, @pcc wrote:

So maybe we should have just one static function:

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That should hopefully make the API harder to misuse.

+1

If we really wanted to fix this with a giant hammer, these APIs shouldn't take strings at all. They should take some type that indicates if the name comes from bitcode or an object file, and we should run the IR mangler on IR names to get the object file names. This would remove the need for the MachO-specific underscore removal hack in computeGUIDPreservedSymbols, which looks like it's trying to map from object file names back to IR names. That code is probably broken on Windows.

[mehdi_amini]

In D31444#713402, @rnk wrote:

In D31444#712724, @pcc wrote:

So maybe we should have just one static function:

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That should hopefully make the API harder to misuse.

+1 as well.

This would remove the need for the MachO-specific underscore removal hack in computeGUIDPreservedSymbols, which looks like it's trying to map from object file names back to IR names.

Yes that's what it does.

That code is probably broken on Windows.

Luckily this component is not used on Windows :)
(it is only part of libLTO)
And I'd really like to deprecate and kill this!

[tejohnson]

In D31444#713417, @mehdi_amini wrote:

In D31444#713402, @rnk wrote:

In D31444#712724, @pcc wrote:

So maybe we should have just one static function:

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That should hopefully make the API harder to misuse.

+1 as well.

I agree that change would help.

Looks like getGlobalIdentifier effectively already does the functionality in getRealLinkageName (but should be refactored to just call getRealLinkageName) - so can you fix this bug by invoking the static getGlobalIdentifier instead? That will make the transition to the above single getGUID static API simpler (since it will take the same parameters).

[inglorion]

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That crossed my mind as well, but it would need 3 pieces of information, and we have only 1 in LTO::GlobalResolution. Plus the type id awkwardness that pcc mentioned. You also still need to deal with IRNames and linkage names. In other words, having everything go through that static method would reduce the number of getGUID methods from 2 to 1, which is good, but what is the proposal for making sure it is invoked correctly?

[tejohnson]

In D31444#713537, @inglorion wrote:

static GUID getGUID(StringRef Name, GlobalValue::LinkageTypes Linkage, StringRef FileName);

That crossed my mind as well, but it would need 3 pieces of information, and we have only 1 in LTO::GlobalResolution. Plus the type id awkwardness that pcc mentioned. You also still need to deal with IRNames and linkage names. In other words, having everything go through that static method would reduce the number of getGUID methods from 2 to 1, which is good, but what is the proposal for making sure it is invoked correctly?

By forcing through a single interface that invokes getGlobalIdentifier under the cover, we can ensure the symbols all go through handling such as getRealLinkageName. It is a bit awkward in places where we don't have a linkage type, but in those cases we could presumably default linkage type to global so that no renaming is done.

[inglorion]

So the idea is to essentially force callers to supply a linkage type and a filename so that they have to think about this stuff, and then say "If you don't have a linkage type, just pass ExternalLinkage and a dummy filename"? And then always call getRealLinkageName inside getGUID to correctly handle IRNames and hope that nobody ever has a non-IR name that starts in "\x01"?

[tejohnson]

In D31444#713580, @inglorion wrote:

So the idea is to essentially force callers to supply a linkage type and a filename so that they have to think about this stuff, and then say "If you don't have a linkage type, just pass ExternalLinkage and a dummy filename"? And then always call getRealLinkageName inside getGUID to correctly handle IRNames and hope that nobody ever has a non-IR name that starts in "\x01"?

That was my thinking on the proposal. I'll let pcc and mehdi chime in on whether that's what they also had in mind.

[mehdi_amini]

This matches also what I had in mind on a high level (didn't look into the details).

[pcc]

In D31444#713580, @inglorion wrote:

So the idea is to essentially force callers to supply a linkage type and a filename so that they have to think about this stuff, and then say "If you don't have a linkage type, just pass ExternalLinkage and a dummy filename"?

Yes.

And then always call getRealLinkageName inside getGUID to correctly handle IRNames and hope that nobody ever has a non-IR name that starts in "\x01"?

The getGUID function should only be taking IR names, never mangled names. To express a mangled name starting with "\x01" you would use something like "\x01\x01foo".

It is unfortunate that we have at least three kinds of names: IR names, getRealLinkageName() and true mangled names as used in object files. I hope we can at least be able to merge the last two in the future, but for now there should be clear comments explaining the different types of names.

[inglorion]

That sounds reasonable; it forces LTO to make explicit that the symbols it's getting GUIDs for are non-local. That leaves type ids, which I don't know much about. I suppose we could make those go through a new getGUIDForTypeId method that does what the static getGUID does now.

[inglorion]

Since refactoring getGUID is going to touch a fair few more places than this diff does, is it OK to do that in a follow-up? I'm still struggling to reduce the test case for this change from "ninja FileCheck" to something smaller, but I really don't want to hold it up any longer than necessary. I have a few more changes that will make Clang build Clang with ThinLTO on Windows, and I'm excited to get them all committed.

[pcc]

In D31444#713610, @inglorion wrote:

Since refactoring getGUID is going to touch a fair few more places than this diff does, is it OK to do that in a follow-up?

Yes, that's fine, thanks.

[tejohnson]

In D31444#713611, @pcc wrote:

In D31444#713610, @inglorion wrote:

Since refactoring getGUID is going to touch a fair few more places than this diff does, is it OK to do that in a follow-up?

Yes, that's fine, thanks.

Right and note I wasn't suggesting doing that in this patch either - rather just preparing for that by changing the places you added calls to getRealLinkageName to instead call getGlobalIdentifier

[inglorion]

clang-format, added test

[pcc]

test/ThinLTO/X86/Inputs/foobar.ll

1 ↗

(On Diff #93574)

Is this input needed? Since it is in a library and nothing refers to its symbols I wouldn't have expected it to be included in the link.

test/ThinLTO/X86/Inputs/qux.ll

1 ↗

(On Diff #93574)

Please give this input a name matching the test case.

test/ThinLTO/X86/mangled.ll

5 ↗

(On Diff #93574)

I would try passing qux.obj directly instead of creating a library, I wouldn't have expected that to make a difference in this case.

[inglorion]

simplified test per @pcc's suggestions (thanks!)

[pcc]

test/ThinLTO/X86/mangled.ll

3 ↗

(On Diff #93697)

Sorry, I missed this before. Because this test uses LLD, it either needs to be moved to LLD's test suite or be rewritten to use llvm-lto2.

[inglorion]

moved the tests to lld (D31549)

[pcc]

LGTM

llvm/trunk/lib/LTO/LTO.cpp

Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
   if (Conf.OptLevel > 0) {
     // Compute "dead" symbols, we don't want to import/export these!
     DenseSet<GlobalValue::GUID> GUIDPreservedSymbols;
     for (auto &Res : GlobalResolutions) {
       if (Res.second.VisibleOutsideThinLTO &&
           // IRName will be defined if we have seen the prevailing copy of
           // this value. If not, no need to preserve any ThinLTO copies.
           !Res.second.IRName.empty())
-        GUIDPreservedSymbols.insert(GlobalValue::getGUID(Res.second.IRName));
+        GUIDPreservedSymbols.insert(GlobalValue::getGUID(
+            GlobalValue::getRealLinkageName(Res.second.IRName)));
     }
 
     auto DeadSymbols =
         computeDeadSymbols(ThinLTO.CombinedIndex, GUIDPreservedSymbols);
 
     ComputeCrossModuleImport(ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
                              ImportLists, ExportLists, &DeadSymbols);
 
     std::set<GlobalValue::GUID> ExportedGUIDs;
     for (auto &Res : GlobalResolutions) {
       // First check if the symbol was flagged as having external references.
       if (Res.second.Partition != GlobalResolution::External)
         continue;
       // IRName will be defined if we have seen the prevailing copy of
       // this value. If not, no need to mark as exported from a ThinLTO
       // partition (and we can't get the GUID).
       if (Res.second.IRName.empty())
         continue;
-      auto GUID = GlobalValue::getGUID(Res.second.IRName);
+      auto GUID = GlobalValue::getGUID(
+          GlobalValue::getRealLinkageName(Res.second.IRName));
       // Mark exported unless index-based analysis determined it to be dead.
       if (!DeadSymbols.count(GUID))
-        ExportedGUIDs.insert(GlobalValue::getGUID(Res.second.IRName));
+        ExportedGUIDs.insert(GUID);
     }
 
     auto isPrevailing = [&](GlobalValue::GUID GUID,
                             const GlobalValueSummary *S) {
       return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
     };
     auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
       const auto &ExportList = ExportLists.find(ModuleIdentifier);