[PATCH] Linker: Replace overridden subprograms

[David Blaikie]

On Tue, Dec 16, 2014 at 2:18 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
>
> > On 2014-Dec-16, at 12:49, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Should "OverridingFunctions" be "OverriddenFunctions"?
>
> Nope, these are the overriding functions.  The functions they've
> overridden have been deleted and no longer exist.
>
> > (it looks like everything in that set gets dropped from the subprogram
> lists - so if those are the functions chosen to be kept by the linker, I
> would think that would end up with them all being dropped from the debug
> info - the exact opposite of what's desired - but I assume I'm
> misunderstanding something)
>
> The timing is important.
>
> When this function is called, `OldF->replaceAllUsesWith(NewF)` has been
> called, so all the old subprograms will be pointing at the NewF.
>

Ah


> However, the named metadata hasn't been linked yet, so the new compile
> units aren't yet in `DstM`.
>

OK. & I take it we always favor/keep the function from SrcM? (what about
cases where we can't - weak in source, strong in destination?)


> >
> > (stylistically I'd remove a bunch of the conditionals in that code as
> not too important: if the function set is empty, everything else should be
> fairly trivial, I don't necessarily know that the early return is
> particularly valuable.
>
> This function gets called each time a module gets linked in.
> Unnecessarily traversing all the compile units and their subprograms is
> wasteful.
>
> Moreover, this code is quadratic in the worst case, since it gets called

once for every module that's linked in (and it traverses all the compile
> units added so far).


Should we wait and do it just once at the end of linking all the modules
in, then?


>   In practice, most `SrcM` won't have functions that
> have overridden something in `DstM`.


Pretty much any C++ module it's going to be true for, I'd imagine -
something as simple as std::string::empty is probably in almost any TU,
eventually, for example (or take some similarly common function).


> Returning early here is important.
>

I realize the counterpoint to premature optimization is don't write silly
inefficient code, but I'd be surprised if any of this showed up in
profiles, though I could be wrong - don't have nearly the
intuition/experience in LTO performance profiles that you do.


> > Two calls to getNamedMetadata (& two checks of the same non-null result)
>
> Actually, these are two different results.  One is for the `SrcM`, and
> the other is for the `DstM`.  If there is no compile unit in the `SrcM`,
> then I'm leaving the subprograms as they are the `DstM`, since there's no
> collision.
>

Right right.


>
> > . CUs in the CU list should never be null, the subprogram array should
> never be null (I think that's the case?).
>
> This might be true... I can change these to assertions.  However
> depending on how I resolve Rafael's concern about symmetry, there could
> be nulls in the array of subprograms.  Do you think that's a problem?
>

Yeah, I'd rather avoid introducing that possibility - I'm pretty sure I've
removed a bunch of conditionals that might've allowed that at some point &
I'd wager we'll assert pretty pervasively if there are null elements in the
subprogram list.


> (Still, I'd rather add the code with conditions and switch it to
> assertions in a separate commit, so that if the assertions fire we know
> it's unrelated to the other semantic changes.)
>

So long as it follows shortly after I don't much mind which way you do this
- I just would really like to avoid being too loose with these things, such
code has hid a miriad of mistakes on a regular basis.


> > & I might even not bother with the old size != new size condition either
> - just to keep the code simple/easy to read)
>
> Calling `replaceSubprograms()` creates a lot of function traffic, and
> `MDNode::get()` isn't guaranteed to return the same `MDNode` (since the
> one there isn't guaranteed to be in uniquing mode).  I'd rather save
> memory and time, since LTO is already bad enough.
>

Again, your call - you know the perf profiles of this stuff & I've no clue.
I'd just be surprised if this showed up.

- David


>
> >
> > - David
> >
> > On Tue, Dec 16, 2014 at 12:14 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2014 Dec 16, at 10:27, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Tue, Dec 16, 2014 at 10:06 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > > > On 2014-Dec-16, at 10:00, David Blaikie <dblaikie at gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, Dec 16, 2014 at 9:48 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
> > > > > It's possible to put DWARF subprogram definitions for linkonce_odr
> functions
> > > > > into comdat groups so they are dropped along with the linkonce_odr
> function,
> > > > > but Clang (& I don't think GCC by default) does that (there's some
> overhead
> > > > > to that representational choice, etc). But yes, it can be done. I
> think
> > > > > today, for both GCC and Clang, you'd end up with two subprogram
> definitions
> > > > > (one in each DWARF compile unit) each pointing to the same
> high_pc/low_pc to
> > > > > describe the function.
> > > >
> > > > I thought this was part of a special case for dwarf in linkers (and
> > > > the missing feature why lld produced binaries are much larger in -g
> > > > builds).
> > > >
> > > > To the best of my knowledge, at least the usual Linux linkers (gold
> and binutils-ld) don't have any special cases for DWARF (well, gold has
> some magic to generate a GDB debug info index, maybe), it's just sections
> and relocations like any other data in an object file. /maybe/ dsymutil on
> MacOS does, but I've not heard about it. (& possibly dwz - a debug
> info-aware compression tool could do that trick, it's designed to eliminate
> redundancy in debug info)
> > > >
> > > > A basic test case shows roughly what I described. (attached
> dwarfdump, if you're curious - you can see what the source would've looked
> like from the DWARF there (inline function in a header, two translation
> units that instantiate the inline function, etc))
> > > >
> > > >
> > > > > (but yeah, vaguely sounds like what you're suggesting would make
> sense - I
> > > > > haven't looked at the initial proposed patch yet, though)
> > > >
> > > > Cheers,
> > > > Rafael
> > > > <dump.txt>
> > >
> > > Even if other linkers don't do it, is it reasonable to delete
> subprograms from
> > > compile units if the canonical one is from another compile unit (and
> has a
> > > subprogram there)?
> > >
> > > I think so - more importantly, I assume that's what we'll end up doing
> anyway. (check the actual DWARF output - since we build a map of
> subprograms, I don't think we end up emitting the subprogram to both CUs
> anyway - we just ignore all but the first one we find - that's my bet
> anyway)
> > >
> > >
> > > If so, I can just do that.
> >
> > How does this look?
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141216/f642a68a/attachment.html>