[PATCH] Module: Use existing library functions

[Justin Bogner]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>> 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.
>
> Actually, I think this use case is well-supported by the current
> behaviour.  The first call will move the function with local linkage and
> insert a function with external linkage.  Subsequent calls will get the
> function with external linkage.
>
> The new behaviour, on the other hand, could cause a problem with this
> use case.

Bah, I don't know what I was thinking. I read the code backwards. The
new behaviour could be *very* bad in this case, as we might start
mucking with a user's function. 

This doesn't seem to be all that common, but the one example I found
that does this actually sets the linkage of the function to internal!
I'm pretty sure the code in question is just buggy though - it probably
shouldn't be using getOrInsertFunction at all. The current behaviour
papers over the problem.

>> 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.
>
> We usually only hit this in LTO.
>
> Another time we could hit this is if someone has defined a version
> of, e.g., puts() in a local file and declared it static.  On inserting
> a call to puts(), the old behaviour would move the static method out of
> the way, while the new behaviour would use the static method.
>
> This could be a problem.  Comments?
>
>> 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

Ignore this, the same confusion from (1) applies and this point doesn't
make any sense.

>> 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.
>
> Anyone else have an opinion?