Page MenuHomePhabricator

[LLD][ThinLTO] Handle GUID collision in import global processing
ClosedPublic

Authored by the_jk on Sep 7 2019, 3:01 PM.

Details

Summary

If there are a GUID collision between two globals checking the
summarylist from the import index to make assumption can be dangerous.

Do not assume that a GlobalValue that has a GlobalVarSummary
actually is a GlobalVariable as it can be another GlobalValue with
the same GUID that the summary is connected to.

Diff Detail

Event Timeline

the_jk created this revision.Sep 7 2019, 3:01 PM
MaskRay added a subscriber: MaskRay.Sep 7 2019, 6:47 PM

Sorry, I somehow missed this. My understanding is that this is an ODR violation and should trigger linker error, instead of trying to be smart.
Adding @tejohnson

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
247

Why this check is needed?

tejohnson added inline comments.Oct 10 2019, 8:50 AM
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
247

I suppose it is because in e.g. the test case the two local F values have the same GUID and we are attempting to mark only the variable in this module.

There is already a facility for doing this - see ModuleSummaryIndex::findSummaryInModule(ValueInfo VI, StringRef ModuleId)

I wonder how such thing happened in the first place. Can linker produce working image even if we circumvent the crash?

I wonder how such thing happened in the first place. Can linker produce working image even if we circumvent the crash?

There isn't an ODR issue since they are both locals. The GUID is the same because, since it is a local, the name is created by prepending the source_filename, which is "-" in both cases, which defeats our normal mechanism for creating a unique GUID for locals. It turned out this was due to the way clang was being invoked by icecc (see the bug for details). It was also addressed separately by D67592, but this is still good to fix.

Is this the only place where bad things can happen? May be simply raise an error in addGlobalValueSummary when new summary type is different from that of SummaryList[0]?

Is this the only place where bad things can happen? May be simply raise an error in addGlobalValueSummary when new summary type is different from that of SummaryList[0]?

We should probably have more testing of this situation. But if we can make it work we should wherever possible.

the_jk updated this revision to Diff 224702.Oct 11 2019, 4:06 PM

Updated to latest master.

Another place that already handles multiple locals with the same GUID is in the same file: bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(), from a comment there:

// When exporting, consult the index. We can have more than one local
// with the same GUID, in the case of same-named locals in different but
// same-named source files that were compiled in their respective directories
// (so the source file name and resulting GUID is the same). Find the one
// in this module.

so I think raising an error would cause surprises for different setups.
I'll gladly add more testcases if you can give me some ideas/tips on possible problems.

Another place that already handles multiple locals with the same GUID is in the same file: bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(), from a comment there:

// When exporting, consult the index. We can have more than one local
// with the same GUID, in the case of same-named locals in different but
// same-named source files that were compiled in their respective directories
// (so the source file name and resulting GUID is the same). Find the one
// in this module.

so I think raising an error would cause surprises for different setups.
I'll gladly add more testcases if you can give me some ideas/tips on possible problems.

I think the testing you have here is good for this patch. But please implement my suggestion to use findSummaryInModule, and also add a comment similar to this one.

the_jk updated this revision to Diff 226383.Fri, Oct 25, 12:55 AM

Sorry, I somehow missed the reference to findSummaryInModule the first time. Updated diff.

tejohnson accepted this revision.Fri, Oct 25, 6:50 AM

LGTM. Small suggestion to expand comment a bit.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
243

Please add note about this being because of same named but different source files compiled without distinguishing path context.

This revision is now accepted and ready to land.Fri, Oct 25, 6:50 AM
the_jk updated this revision to Diff 226427.Fri, Oct 25, 7:20 AM

Include all of the comment.

I don't have commit access so I'd need someone to push this, thanks.

I don't have commit access so I'd need someone to push this, thanks.

I'll take care of that for you today.

I don't have commit access so I'd need someone to push this, thanks.

I'll take care of that for you today.

Actually, I don't believe I have commit access at the moment either (didn't add my git username before the migration this week). I'll commit this as soon as I can get access...

This revision was automatically updated to reflect the committed changes.
tejohnson reopened this revision.Fri, Nov 1, 9:58 AM

I had to revert this as it was causing a failure in my testing. Looking into it...

This revision is now accepted and ready to land.Fri, Nov 1, 9:58 AM
tejohnson closed this revision.Fri, Nov 1, 1:58 PM

Recommitted in 16ec00eee7e348767f4393f189044f87f6374031 with fix for the failure I saw.