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

[Reid Kleckner via llvm-commits]

rnk added a comment.

In http://reviews.llvm.org/D21255#458935, @jlebar wrote:

> 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.


Yeah, it is pretty crummy. I think this is why David didn't copy the comdat with the other attributes in the first place.

I like the idea of asserting in setParent. We don't currently do the analogous check for GlobalVariable initializers, but we should consider it. :)


http://reviews.llvm.org/D21255