[PATCH] Module: Use existing library functions
mail at justinbogner.com
Tue Mar 4 01:37:28 PST 2014
"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?
More information about the llvm-commits