This is an archive of the discontinued LLVM Phabricator instance.

[IR] Use Min behavior for module flag "PIC Level"
ClosedPublic

Authored by MaskRay on Jul 25 2022, 4:48 PM.

Details

Summary

Using Max for both "PIC Level" and "PIE Level" is inconsistent. PIC imposes less
restriction while PIE imposes more restriction. The result generally
picks the more restrictive behavior: Min for PIC.

This choice matches ld -r: a non-pic object and a pic object merge into a
result which should be treated as non-pic.

To allow linking "PIC Level" using Error/Max from old bitcode files, upgrade
Error/Max to Min.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 25 2022, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 4:48 PM
MaskRay requested review of this revision.Jul 25 2022, 4:48 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

@tmsriram can you take a look? I can review from a code standpoint but you are more familiar with PIC/PIE details.

Looked into this a bit and the change does also seem correct to me. One comment: I see a test checking the upgrade behavior from the old Error type, but not for the upgrade from Max to Min.

MaskRay updated this revision to Diff 453806.Aug 18 2022, 3:00 PM

add Bitcode/upgrade-pic-level.ll

This revision is now accepted and ready to land.Aug 18 2022, 3:51 PM
This revision was landed with ongoing or failed builds.Aug 18 2022, 4:29 PM
This revision was automatically updated to reflect the committed changes.

I'm not understanding why this doesn't also apply to "PIE Level", doesn't it also follow the same reasoning? pic -> PIC is the same as pie -> PIE

e.g. if you merge a small PIC and large PIC file, the resulting file would only be guaranteed to work with a "large PIC executable" (unsure what the right term is) and not a "small PIC executable", so if we say it's a large PIC file, that's wrong since it wouldn't link into a "large PIC executable", so we have to conservatively say it's a small PIC file.
and s/PIC/PIE for the same argument

MaskRay added a comment.EditedSep 12 2023, 2:39 PM

I'm not understanding why this doesn't also apply to "PIE Level", doesn't it also follow the same reasoning? pic -> PIC is the same as pie -> PIE

e.g. if you merge a small PIC and large PIC file, the resulting file would only be guaranteed to work with a "large PIC executable" (unsure what the right term is) and not a "small PIC executable", so if we say it's a large PIC file, that's wrong since it wouldn't link into a "large PIC executable", so we have to conservatively say it's a small PIC file.
and s/PIC/PIE for the same argument

"PIE Level" is a bit of a misdesign. We should treat the value as a boolean and ignore 1/2 difference.
"PIC Level" decides the small PIC vs large PIC difference, as well as the small PIE vs large PIE difference.

If we merge an -fno-pic/-fpie IR with a -fpic IR, the output cannot be linked with -shared, so we should merge false "PIE Level" and true "PIE Level" to true.