This is an archive of the discontinued LLVM Phabricator instance.

[IR] Disallow llvm.global_ctors and llvm.global_dtors of the 2-field form in textual format
ClosedPublic

Authored by MaskRay on May 4 2019, 1:08 AM.

Details

Summary

The 3-field form was introduced by D3499 in 2014 and the legacy 2-field
form was planned to be removed in LLVM 4.0

For the textual format, this patch migrates the existing 2-field form to
use the 3-field form and deletes the compatibility code.
test/Verifier/global-ctors-2.ll checks we have a friendly error message.

For bitcode, lib/IR/AutoUpgrade UpgradeGlobalVariables will upgrade the
2-field form (add i8* null as the third field).

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.May 4 2019, 1:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2019, 1:08 AM
rnk added a comment.May 6 2019, 11:03 AM

I'm concerned that this will break most frontends for LLVM, and I don't recall ever making any attempt to document this in the release notes or do any kind of outreach at all. LLVM doesn't promise API stability, so in some ways this is normal, but this is more surprising because it may lead to harder to understand runtime errors, especially if the verifier is disabled.

Can you turn the old auto-upgrade test into a verifier test that checks for an improved diagnostic? I think that would strike the right balance between removing legacy and not needlessly breaking people.

lib/IR/Verifier.cpp
647 ↗(On Diff #198128)

Please improve this diagnostic since it is very likely to be the first time frontend authors find out about the third field.

The 3-field form was introduced by D3499 in 2014 and the legacy 2-field
form was planned to be removed in LLVM 4.0

Note that "remove in LLVM 4.0" was aspirational, at a time when some community members thought we'd break bitcode compatibility in 4.0, like we did in 3.0. But that didn't come to pass. Please confirm we have a test in place already that tests we do keep bitcode compatibility. I also suggest adding a release note.

MaskRay updated this revision to Diff 198462.May 7 2019, 7:10 AM

Bring back test/Bitcode/upgrade-global-ctors.ll to check we emit the error:
the third field of the element type is mandatory, specify i8* null to migrate from the obsoleted 2-field form

Add a bullet point to docs/ReleaseNotes.rst "Changes to the LLVM IR"

MaskRay marked an inline comment as done.May 7 2019, 7:11 AM
dexonsmith requested changes to this revision.May 7 2019, 10:41 AM

Hmm; it doesn't seem safe to obsolete the 2-field form if we don't have a working bitcode upgrade from 2-field to 3-field.

test/Bitcode/upgrade-global-ctors.ll
4 ↗(On Diff #198462)

It seems like you might need to reimplement a bitcode upgrade here... why was it disabled?

7 ↗(On Diff #198462)

I don't think this is acceptable when reading from bitcode.

This revision now requires changes to proceed.May 7 2019, 10:41 AM
MaskRay updated this revision to Diff 198583.May 7 2019, 9:45 PM
MaskRay retitled this revision from Disallow llvm.global_ctors and llvm.global_dtors of the 2-field form to [IR] Disallow llvm.global_ctors and llvm.global_dtors of the 2-field form in textual format.
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a reviewer: dexonsmith.

Implement autograde. only disallow textual format

MaskRay marked 3 inline comments as done.May 7 2019, 9:45 PM
MaskRay added inline comments.
test/Bitcode/upgrade-global-ctors.ll
7 ↗(On Diff #198462)

Implemented the autograde code and improved the test a bit.

MaskRay updated this revision to Diff 198584.May 7 2019, 9:47 PM
MaskRay marked an inline comment as done.

Delete an unused variable

rnk added a comment.May 8 2019, 9:28 AM

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

Agreed, the existing file is what we want to test.

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

What's the problem with using the pre-existing file?

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

What's the problem with using the pre-existing file?

test/Bitcode/upgrade-global-ctors.ll.bc uses zeroinitializer, it doesn't check if AuoUpgrader adds i8* null as the third field. So I improved the test a bit and made it clear both globals_ctors and global_dtors are upgraded.

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

What's the problem with using the pre-existing file?

test/Bitcode/upgrade-global-ctors.ll.bc uses zeroinitializer, it doesn't check if AuoUpgrader adds i8* null as the third field. So I improved the test a bit and made it clear both globals_ctors and global_dtors are upgraded.

It makes sense to check global_dtors, but changing binary files is expensive in Git, and it's important to use a known version of llvm-as to generate the bitcode file. Here's what I suggest.

  • Leave test/Bitcode/upgrade-global-ctors.ll.bc alone (revert to the previous binary).
    • Update test/Bitcode/upgrade-global-ctors.ll's CHECK line appropriately.
    • Put the verify-uselistorder checks back into the test.
  • Add a new test for the destructor, probably test/Bitcode/upgrade-global-dtors.ll{,.bc}.
    • Generate the .bc using llvm-as from the most recent LLVM release (8.0, right?) and document in the .ll how it was generated and with what version.
    • Add verify-uselistorder checks.
MaskRay updated this revision to Diff 198979.May 9 2019, 10:24 PM

Restore upgrade-global-ctors.ll.bc
Add upgrade-global-ctors.ll.bc assembled by llvm-dis 8.0.0

rnk accepted this revision.May 14 2019, 1:31 PM

lgtm, thanks for adding the diagnostic!

This revision is now accepted and ready to land.May 14 2019, 1:31 PM
dexonsmith accepted this revision.May 14 2019, 1:59 PM

LGTM too.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/Instrumentation/MemorySanitizer/global_ctors_2to3.ll