[PATCH] Module: Use existing library functions

[Justin Bogner]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
> During LTO, user-supplied definitions of C library functions often
> exist.  -instcombine uses Module::getOrInsertFunction() to get a handle
> on library functions (e.g., @puts, when optimizing @printf).
>
> Previously, Module::getOrInsertFunction() would rename any matching
> functions with local linkage, and create a new declaration.  In LTO,
> this is the opposite of desired behaviour, as it skips by the
> user-supplied version of the library function and creates a new
> undefined reference which the linker often cannot resolve.

Why do the user-supplied versions have local linkage? That seems a bit
odd to me, but I'm not all that familiar with the details of LTO.

> Removing that logic did not cause any tests to fail, and I couldn't
> think of why it's *good* behaviour.  Maybe no one was using it?

As far as I can tell there are three use cases for getOrInsertFunction:

1. Lazily creating a function that we want to manipulate - the first
   call of getOrInsertFunction inserts, and subsequent calls get. The
   "insert" logic creates a function with ExternalLinkage, so the
   removed code path shouldn't occur for this usage. On the other hand,
   if someone is attempting to do this and a function with local linkage
   already exists with that name, the result will be completely broken,
   since we'll get a large number of copies of the function we intended
   to manipulate.

2. Accessing a function with a well known name, which expect will
   already exist. Creating a function here doesn't make much sense, so
   the renaming behaviour also seems broken, but it's surprising to me
   that such a function would have local linkage, as above, so I don't
   see why we hit this.

3. Creating a unique function. In this came I guess the name is only
   there to help with debugging, so the behaviour we currently have kind
   of makes sense. This is a bit silly though, since the "get" part of
   getOrInsertFunction would never be wanted. I'd expect callers that
   need this (if there are any) to just do Function::Create

So basically, I can't think of how this would be good behaviour either,
and there are at least two ways in which it's bad.

> I added two testcases: one specifically for the -instcombine behaviour
> and one for the LTO flow that exposed it.