Page MenuHomePhabricator

[llvm-link] Fix for an assertion when linking global with appending linkage
ClosedPublic

Authored by sdmitriev on Jan 21 2021, 4:00 AM.

Details

Summary

This patch fixes llvm-link assertion when linking external variable
declaration with a definition with appending linkage.

Diff Detail

Event Timeline

sdmitriev created this revision.Jan 21 2021, 4:00 AM
sdmitriev requested review of this revision.Jan 21 2021, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 4:00 AM

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern. And what happens before/now if you have append on both @var? We should add those tests if possible.

llvm/lib/Linker/IRMover.cpp
941

parenthesis around the condition please. maybe move it to a new line given the complexity.

sdmitriev updated this revision to Diff 318436.Jan 22 2021, 1:24 AM
sdmitriev marked an inline comment as done.

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern.

On current implementation llvm-link will die with an assertion no matter if extern is there or not. With this (updated) patch llvm-link will print an error message that both variables should have appending linkage (if extern is removed). I have added such test as well as few other negative tests.

And what happens before/now if you have append on both @var? We should add those tests if possible.

That should be fine, arrays from both vars will be appended in the result as described in the langref. This behavior is already checked by the following tests - Linker/AppendingLinkage.ll, Linker/AppendingLinkage2.ll.

jdoerfert accepted this revision.Jan 22 2021, 7:43 AM

Looks reasonable to me. Test coverage is nice now, thanks.

This revision is now accepted and ready to land.Jan 22 2021, 7:43 AM