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

[Rafael Espíndola via llvm-commits]

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

The thing that is odd with the change is that type names should not be
significant. The existing code that uses names is a hack that will
hopefully go away when we get typeless pointers.

In some sense this patch would add more dependency on names. Note that
if the src module types are named T3/T4 they still get mapped to T1.
So what this patch does is prevent renaming of a type that has the
same name in src and dst.

> 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

Where is the getTypeByName? That is the real issue. Since you are
mapping visible (non-local) functions the name of the functions is
significant. Can't you instead get the function in the linked module
and from there get the type? That way you don't depend no the name of
the type.

Cheers,
Rafael