Page MenuHomePhabricator

LowerTypeTests: Propagate symver directives
ClosedPublic

Authored by vlad.tsyrklevich on Apr 18 2018, 10:49 PM.

Details

Summary

This change fixes https://crbug.com/834474, a build failure caused by
LowerTypeTests not preserving .symver symbol versioning directives for
exported functions. Emit symver information to ThinLTO summary data and
then propagate symver directives for exported functions to the merged
module.

Emitting symver information to the summaries increases the size of
intermediate build artifacts for a Chromium build by less than 0.2%.

Diff Detail

Event Timeline

lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
361

@pcc This should probably be F->hasAddressTaken() instead of F->use_empty(), but I seem to recall that that would be actually be to restrictive for which functions we currently export--is that right?

pcc accepted this revision.Apr 19 2018, 11:31 AM
pcc added a subscriber: tejohnson.

LGTM

I don't think this will do the right thing if two modules have different .symver targets, but then again that would seem to be problematic even without CFI (consider the case where the modules with different targets get imported into one another).

Probably something to consider in the future would be to rename functions at the IR level in the ThinLTOBitcodeWriter according to the .symver directives.

lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
361

I think so, in the importer we would end up redirecting all references (including direct calls) to go through the jump table, so we need to make sure that the jump table points to the right function.

This revision is now accepted and ready to land.Apr 19 2018, 11:31 AM

I started thinking about renaming but it's complicated by the fact that there are different usages of the symver directive that slightly change the renaming logic. For now I'm going to keep it as is but I've also submitted https://reviews.llvm.org/D45845 to make sure that builds fail correctly if multiple mismatched symvers are emitted.

This revision was automatically updated to reflect the committed changes.