<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 22, 2016 at 7:17 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">> Also is it even correct to use the original linkage as-is in all cases? Should it be ExternalLinkage?<br>
<br>
</span>Good catch. I was expecting the verifier to catch out of comdat<br>
references to locals, but it doesn't.<br>
<span class=""><br>
> Finally, what if this is ThinLTO and the alias was defined in the original module we are importing into? Won't we have an undef at link time? Ditto for full LTO - where would this new declaration be defined? I'm not even sure what the right behavior is in that case.<br>
<br>
<br>
</span>With comdats the same symbols have to be defined in the comdat group<br>
we selected. If that is not the case, an undefined error is the<br>
correct result. The ELF wording is:<br></blockquote><div><br></div><div>Ok, I wasn't sure (and because this handling is for COFF not ELF). It might be worth adding a comment to this effect where you are doing the alias conversion to a declaration. Especially since the new test case has exactly this situation: e.g. @alias, @func and @var etc are in one comdat but not the selected one.</div><div><br></div><div>Then it sounds like normally we would expect that any alias which is dropped would also have to be defined in the source module comdat group, so the declaration created here would be replaced by the def from the selected imported group from the source copy? If it isn't replaced with a def, then if the comdat members are linkonce the defs might be dropped from the source module when it is compiled.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
-------------------------------------------------<br>
A symbol table entry with STB_GLOBAL or STB_WEAK binding that is<br>
defined relative to one of a group's sections, and that is contained<br>
in a symbol table section that is not part of the group, must be<br>
converted to an undefined symbol (its section index must be changed to<br>
SHN_UNDEF) if the group members are discarded. References to this<br>
symbol table entry from outside the group are allowed.<br>
-------------------------------------------------<br></blockquote><div><br></div><div>Does "A symbol table entry ... defined relative to one of a group's sections" refer to any alias to a member of a comdat group? I'm having a hard time parsing this and understanding which situation it is describing, since I would expect such an alias to be in the same symbol table section as the rest of the group. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> ================<br>
> Comment at: lib/Linker/LinkModules.cpp:479<br>
> @@ +478,3 @@<br>
> + GlobalValue *Declaration =<br>
> + new GlobalVariable(M, Ty.getElementType(), /*isConstant*/ false, L,<br>
> + /*Initializer*/ nullptr);<br>
> ----------------<br>
> What if this was a function alias not variable alias?<br>
<br>
</span>That is fine. It is a bit odd that we have declaration versions of 2<br>
of the 3 GlobalObjects (there should be only 1 IMHO), but the linker<br>
can handle a mismatch.<br></blockquote><div><br></div><div>So you are saying that this will produce a declaration that the compiler/linker is ok with, even if the aliasee is a function? That seems a bit funky to do regardless. I see another place where we drop symbols that creates the right kind of object type for the new declaration based on the aliasee here: See the alias handling in runOnModule() in lib/Transforms/IPO/ExtractGV.cpp. Maybe that handling can be refactored into a helper that could be invoked here...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> ================<br>
> Comment at: lib/Linker/LinkModules.cpp:504<br>
> @@ +503,3 @@<br>
> + bool LinkFromSource = P.second.second;<br>
> + if (!LinkFromSource)<br>
> + continue;<br>
> ----------------<br>
> Why not combine this into the above loop? I.e. when the ComdatsChosen array is initialized for C, you already have its LinkFromSrc. And that step is done exactly once for each comdat due to the earlier check that continues if C is already in ComdatsChosen.<br>
<br>
</span>Excellent suggestion. Thanks!<br>
<br>
I will upload a new patch in a sec.<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>