<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 23, 2016 at 1:15 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 23 March 2016 at 16:06, Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>> wrote:<br>
> tejohnson added a comment.<br>
><br>
> I'm not sure how these changes are fixing the issue described - we used to always add to ValuesToLink any GV in a chosen comdat, but here we only sometimes do. Can't that result in incomplete comdat groups?<br>
<br>
</span>No. The idea is that dropping a comdat will drop a symbol. Not<br>
dropping it just means it goes to normal symbol resolution, not that<br>
it *must* be included.<br>
<br>
So, if we don't drop a comdat, we will still look at every symbol in<br>
that comdat and not get an incomplete group. It is possible that we<br>
will decide to use an existing symbol instead, but that is fine.<br></blockquote><div><br></div><div>I see, if we go to the shouldLinkFromSource then we had an existing symbol, so we are guaranteed to get a copy of each symbol and therefore a complete group.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> I tried the new test case with HEAD and see that it doesn't have a $c1 comdat at all, but not sure why or how that is fixed by the patch. I'm obviously missing something, can you give me a better idea of how this is working?<br>
<br>
</span>At head we force the IRMover to keep (for example) the @v1 from<br>
Inputs/comdat16.ll. Since these symbols have the c2 comdat, the c1<br>
comdat in the result is empty and is dropped.<br></blockquote><div><br></div><div>Ok, got it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
> ================<br>
> Comment at: lib/Linker/LinkModules.cpp:425<br>
> @@ -424,4 +424,3 @@<br>
>      std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];<br>
> -    if (LinkFromSrc)<br>
> -      ValuesToLink.insert(&GV);<br>
> -    return false;<br>
> +    if (!LinkFromSrc)<br>
> +      return false;<br>
> ----------------<br>
> We now only add comdat members to ValuesToLink on a case by case basis below.<br>
<br>
</span>Yes. That is the main part of the patch. A comdat can drop parts of a<br>
file, it doesn't force the symbol resolution of what is left.<br>
<span class=""><br>
> ================<br>
> Comment at: lib/Linker/LinkModules.cpp:567<br>
> @@ -567,2 +566,3 @@<br>
>      for (GlobalValue *GV2 : ComdatMembers[SC])<br>
> -      ValuesToLink.insert(GV2);<br>
> +      if (GV2->hasInternalLinkage())<br>
> +        ValuesToLink.insert(GV2);<br>
> ----------------<br>
> Why only internal members? If we have a comdat member in ValuesToLink (added in linkIfNeeded), then presumably we had selected that copy of the comdat and wouldn't we want all members?<br>
<br>
</span>The correct thing is to drop this entire loop. I indent to do that in<br>
a followup patch.<br></blockquote><div><br></div><div>I see now why we need to add internal members to the ValuesToLink set here - when we selected the dest copy of that particular symbol but need to keep a local (will be renamed) copy of the source symbol for references, right?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
> ================<br>
> Comment at: test/Linker/Inputs/comdat16.ll:5<br>
> @@ +4,3 @@<br>
> +; This is only present in this file. The linker will keep $c1 from the first<br>
> +; file and this will be undefined.<br>
> +@will_be_undefined = global i32 1, comdat($c1)<br>
> ----------------<br>
> Should test check that?<br>
<br>
</span>I will add that.<br>
<br>
Thanks,<br>
Rafael<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>