This is an archive of the discontinued LLVM Phabricator instance.

[llvm-link] Fix crash when materializing appending global
ClosedPublic

Authored by sdmitriev on Jan 24 2021, 10:07 PM.

Details

Summary

This patch fixes llvm-link crash when materializing global variable
with appending linkage and initializer that depends on another
global with appending linkage.

Diff Detail

Event Timeline

sdmitriev created this revision.Jan 24 2021, 10:07 PM
sdmitriev requested review of this revision.Jan 24 2021, 10:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2021, 10:07 PM
tra added inline comments.Jan 25 2021, 9:22 AM
llvm/lib/Transforms/Utils/ValueMapper.cpp
822–824

Does the crash happen because mapAppendingVariable attempts to iterate over an array that may change by the loop?
If that's the case, the fix (making a temp copy with relevant items) should probably be done there.

sdmitriev added inline comments.Jan 25 2021, 10:17 AM
llvm/lib/Transforms/Utils/ValueMapper.cpp
822–824

For the IR from test mapAppendingVariable call causes a new item to be added to the AppendingInits vector, but this item gets incorrectly cleaned up from it by the resize call which follows mapAppendingVariable. As a result we are getting PrefixSize == -1 while processing the second appending global which leads to a crash.

tra added inline comments.Jan 25 2021, 10:36 AM
llvm/lib/Transforms/Utils/ValueMapper.cpp
822–824

I see. I'd rename Inits -> NewInits or UnmappedInits to make it more clear that we only want to process new entries.
Perhaps PrefixSize could also use a more descriptive name, too. NumMappedInits ?

825

What's the typical number of inits do you expect to see in practice?
Presumably, it's smaller than that of AppendingInits itself, which is 16.

sdmitriev updated this revision to Diff 319173.Jan 25 2021, 5:24 PM

Addressed review comments.

sdmitriev added inline comments.Jan 25 2021, 5:26 PM
llvm/lib/Transforms/Utils/ValueMapper.cpp
825

I have renamed Inits to NewInits in the updated patch and reduced its size to 8.

tra accepted this revision.Jan 25 2021, 5:47 PM
This revision is now accepted and ready to land.Jan 25 2021, 5:47 PM