This is an archive of the discontinued LLVM Phabricator instance.

OpaquePtr: Add type to sret attribute
ClosedPublic

Authored by arsenm on Sep 24 2020, 9:41 AM.

Details

Summary

Make the corresponding change that was made for byval in
b7141207a483d39b99c2b4da4eb3bb591eca9e1a. Like byval, this requires a
bulk update of the test IR tests to include the type before this can
be mandatory.

Diff Detail

Event Timeline

arsenm created this revision.Sep 24 2020, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 9:41 AM
arsenm requested review of this revision.Sep 24 2020, 9:41 AM
dblaikie accepted this revision.Sep 24 2020, 10:16 AM

Looks good - thanks!

This revision is now accepted and ready to land.Sep 24 2020, 10:16 AM
rsmith added a subscriber: rsmith.Sep 28 2020, 4:36 PM

This change has introduced crashes into code using the LLVM module linker. I think there are two different bugs here (both appear to be pre-existing infrastructure bugs) that combine to result in the crash.

Bug 1: the LLVM IR linker does not properly link together types inside sret attributes. Testcase:

$ cat a.ll
%a = type {i64}
define void @f(%a* sret(%a)) { ret void }

$ cat b.ll
%a = type {i64}
define void @g(%a* sret(%a)) { ret void }

$ ./build/bin/llvm-link a.ll b.ll -S -o -
; ModuleID = 'llvm-link'
source_filename = "llvm-link"

%a = type { i64 }

define void @f(%a* sret(%a) %0) {
  ret void
}

define void @g(%a* sret(%"type 0x355d870") %0) {
  ret void
}

Note that the two types %a were not properly unified within the sret attribute of @g's parameter.

Bug 2: the value enumerator used by IR printing and serialization does not visit types inside attributes. As seen above, this results in wrong output from the IR printer (note that the printing of %"type 0x355d870" is bogus: that type is not defined anywhere), and results in crashes in the bitcode writer. This crash occurs any time the type in an sret doesn't occur elsewhere in the same Module. Example of this bug in isolation:

$ cat c.ll
define void @f(i8* sret({i64})) { ret void }
$ ./build/bin/opt - -o /tmp/tmp.ir
<crash>

I appreciate that this is a large patch and reverting it might introduce a lot of churn, but this is blocking us, so I'll need to revert if this can't be fixed soon. Thanks!

This change has introduced crashes into code using the LLVM module linker. I think there are two different bugs here (both appear to be pre-existing infrastructure bugs) that combine to result in the crash.

Bug 1: the LLVM IR linker does not properly link together types inside sret attributes. Testcase:

$ cat a.ll
%a = type {i64}
define void @f(%a* sret(%a)) { ret void }

$ cat b.ll
%a = type {i64}
define void @g(%a* sret(%a)) { ret void }

$ ./build/bin/llvm-link a.ll b.ll -S -o -
; ModuleID = 'llvm-link'
source_filename = "llvm-link"

%a = type { i64 }

define void @f(%a* sret(%a) %0) {
  ret void
}

define void @g(%a* sret(%"type 0x355d870") %0) {
  ret void
}

Note that the two types %a were not properly unified within the sret attribute of @g's parameter.

Bug 2: the value enumerator used by IR printing and serialization does not visit types inside attributes. As seen above, this results in wrong output from the IR printer (note that the printing of %"type 0x355d870" is bogus: that type is not defined anywhere), and results in crashes in the bitcode writer. This crash occurs any time the type in an sret doesn't occur elsewhere in the same Module. Example of this bug in isolation:

$ cat c.ll
define void @f(i8* sret({i64})) { ret void }
$ ./build/bin/opt - -o /tmp/tmp.ir
<crash>

I appreciate that this is a large patch and reverting it might introduce a lot of churn, but this is blocking us, so I'll need to revert if this can't be fixed soon. Thanks!

Sorry, should've been some followup here, but there is a fix under review here: https://reviews.llvm.org/D88423

rsmith added a comment.EditedSep 28 2020, 4:43 PM

Sorry, should've been some followup here, but there is a fix under review here: https://reviews.llvm.org/D88423

Looks like that's fixing only Bug 2 and not Bug 1, right? (And fixing Bug 1 would invalidate the testcase.) Or is the value enumerator also used by the module linker?

Sorry, should've been some followup here, but there is a fix under review here: https://reviews.llvm.org/D88423

Looks like that's fixing only Bug 2 and not Bug 1, right? (And fixing Bug 1 would invalidate the testcase.) Or is the value enumerator also used by the module linker?

Ah, quite possibly - I'm not rightly sure. Will pick it up over on the other review.

tpopp added a subscriber: tpopp.Sep 29 2020, 1:33 AM

I have reverted this change based on the comments in the fixup patch. I apologize for any difficulties this causes.

Thanks @tpopp, that'll unblock all of us.

Apologies for not following up on this thread earlier; I figured mentioning this review in the other one would be sufficient, but that isn't obvious here (and I don't know if people got a notification for that).

As mentioned on the other thread, D88423 fixes *only* the bitcode writer part of bug 2, but not the IR printing half, and better test cases are floating around now too so I'll abandon that patch. @arsenm I'm happy to help review and test the next round of this patch.