[PATCH] D88241: OpaquePtr: Add type to sret attribute

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88241/new/

https://reviews.llvm.org/D88241