Page MenuHomePhabricator

[IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level
ClosedPublic

Authored by steven_wu on Aug 9 2017, 4:08 PM.

Details

Summary

From r303590, ModuleFlagBehavior for PIC and PIE level is changed from
Error to Max. This will cause bitcode compatibility issue when linking
against a bitcode static archive built with old compiler.
Add an auto-ugprade path to upgrade the the ModuleFlagBehavior in the
old bitcode to match the new one so IRLinker can link them.

Diff Detail

Repository
rL LLVM

Event Timeline

steven_wu created this revision.Aug 9 2017, 4:08 PM
mehdi_amini edited edge metadata.Aug 9 2017, 10:40 PM
mehdi_amini added a subscriber: hans.
mehdi_amini added a subscriber: hansw.

Is this something that is affecting clang-5.0 backward compatibility with clang-4.0?
(adding @hansw for visibility)

Yes, this will affect clang-5.0 backward compatibility. Thanks for bringing this up. I was planning to nominate to merge into release branch after this gets reviewed.

hans removed a subscriber: hansw.Aug 10 2017, 10:02 AM

Thanks! It's on my radar now.

dexonsmith edited edge metadata.Aug 17 2017, 11:48 AM

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

I don't see a general test for PIE level. Can we add one of those as well?

lib/IR/AutoUpgrade.cpp
2269–2270 ↗(On Diff #110489)

This would read more nicely as auto *Behavior, since you've already been explicit about the ConstantInt.

2273–2274 ↗(On Diff #110489)

It's not generally sound to call MDNode::replaceOperandWith, unless the nodes are "distinct". I strongly suspect these nodes are "uniqued", which means that they are semantically constants (with far-too-lenient an API).

Instead, you should construct a new node to attach to the NamedMDNode.

tejohnson accepted this revision.Aug 17 2017, 11:49 AM

Sorry for the delay, I was out all last week and missed this one. LGTM but with one question below about a test change.

test/Linker/module-flags-pic-1-a.ll
5 ↗(On Diff #110489)

Does this need to change - i.e. with this patch shouldn't it auto-upgrade?

This revision is now accepted and ready to land.Aug 17 2017, 11:49 AM
tejohnson requested changes to this revision.Aug 17 2017, 11:57 AM

Duncan has a good point about replaceOperandWith, please address that (updating to remove patch accept).

This revision now requires changes to proceed.Aug 17 2017, 11:57 AM

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

I don't see a general test for PIE level. Can we add one of those as well?

The PIE test is in the same module.

test/Linker/module-flags-pic-1-a.ll needs to updated because the auto upgrade only happens when you load from bitcode. The texture format is not auto upgradeable and the old value still has valid syntax.

lib/IR/AutoUpgrade.cpp
2273–2274 ↗(On Diff #110489)

I am pretty sure the MDNode added to ModuleFlags are all uniqued.

I am trying to modify the ModuleFlag here. Attaching new node to module flag is easy but removing is not. Sure, I can reconstruct the entire module flag NamedMDNode, but I am not sure if that is worthwhile if the MDNode is ensured to be uniqued.

steven_wu updated this revision to Diff 111754.Aug 18 2017, 2:57 PM
steven_wu edited edge metadata.

Here is a version that rebuilds the module flags to avoid replaceOperandsWith.

dexonsmith requested changes to this revision.Aug 18 2017, 6:02 PM
dexonsmith added inline comments.
lib/IR/AutoUpgrade.cpp
2305–2311 ↗(On Diff #111754)

I think you should just change ModFlags->getOperand(I) in place. I think you'll need to add some API to NamedMDNode, but that should be straightforward.

This revision now requires changes to proceed.Aug 18 2017, 6:02 PM
steven_wu updated this revision to Diff 112005.Aug 21 2017, 9:49 AM
steven_wu edited edge metadata.

Thanks for the feedback. Here is another attempt to modify the module flag in place.

steven_wu updated this revision to Diff 112006.Aug 21 2017, 9:51 AM

Missing one fixup from the previous update. Try again.

dexonsmith accepted this revision.Aug 21 2017, 2:18 PM

LGTM! Thanks for reworking this.

This revision was automatically updated to reflect the committed changes.