[PATCH] D18683: Fix bug with duplicate struct types in Linker

[Tim Armstrong via llvm-commits]

timarmstrong added a comment.

Thanks for taking a look. I agree this a corner case that doesn't matter for ahead-of-time compilation but it's very useful for JIT compilation and it did historically work on older LLVM versions.


================
Comment at: lib/Linker/IRMover.cpp:1480
@@ -1481,2 +1479,3 @@
 
 bool IRMover::IdentifiedStructTypeSet::hasType(StructType *Ty) {
+  return AllStructTypes.count(Ty);
----------------
Another argument for making this change is that 'hasType()' would lie about whether the type was present. I.e. I added an assert like the one below and it would fire if there were two structurally identical types in the module.


```
addNonOpaque(Ty);
assert(hasType(Ty));
```

================
Comment at: unittests/Linker/LinkModulesTest.cpp:346
@@ +345,3 @@
+
+  // Check that we can call both callee1 and callee2 still.
+  // Since T1 and T2 were defined in Src, it should be valid to call functions
----------------
rafael wrote:
> rafael wrote:
> > Looks like this got out of date.There are no calls in the IR in this test.
> Sorry, I now noticed that you create the calls after linking.
> 
> Can the calls be there before?
For context, we have a database that lets users compile user-defined functions to LLVM IR then use them in their queries. We want to be able to link in the user-provided module then generate calls to functions in that module. This worked fine on LLVM 3.3 and this seems like the natural way to do things in a JIT (link in the code, then generate more IR and add it to the module as needed).

It's not all that feasible to restructure the code so that all calls are generated before linking happens. We could do something like generate declarations for all the functions in the module before linking to force it to preserve the types, but it seems a bit unnecessary. When I read through the linker code it wasn't totally clear why that would work but the pattern in the test shouldn't - I was concerned it was working by coincidence rather than by design.

If you're interested in the context, we link in the module from a bitcode file here:

https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/exprs/scalar-fn-call.cc#L142

Then emit calls to functions as needed.
https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/exprs/scalar-fn-call.cc#L466


http://reviews.llvm.org/D18683