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.
Details
Diff Detail
Event Timeline
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
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?
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.