[PATCH] D21255: Fix cloning GlobalValues with comdats across Modules.

[Justin Lebar via llvm-commits]

jlebar added a comment.

In http://reviews.llvm.org/D21255#458782, @rnk wrote:

> Anything wrong with forcing the cross-module use case to be a little more complicated?


I guess the suggestion is,

- leave the implementation of copyAttributesFrom alone, and
- add a comment explaining what needs to be done if you're copying between modules?

I guess I'm fine with that, although given that we already had a nontrivial bug (that took me half a day to track down), I am a bit concerned about leaving the footgun there as-is.  Even if all of the code in llvm is correct, this seems like a pretty sharp edge to expose to future authors.

What if we modified Function::setParent to either

- assert that the comdat is from the right module, or
- fix up the comdat?

I guess if cross-module copiers have to fix up the initializers, an assertion might be the most appropriate thing, but I'm fine with either.


http://reviews.llvm.org/D21255