
<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 18, 2016, at 7:36 PM, Teresa Johnson <<a href="mailto:tejohnson@google.com" class="">tejohnson@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Mon, Jul 18, 2016 at 7:02 PM, Xinliang David Li<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:davidxl@google.com" target="_blank" class="">davidxl@google.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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;"><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote"><span class="">On Mon, Jul 18, 2016 at 5:37 PM, Mehdi AMINI<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:mehdi.amini@apple.com" target="_blank" class="">mehdi.amini@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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;">mehdi_amini added a comment.<br class=""><span class=""><br class="">In<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D22356#487800" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D22356#487800</a>, @tejohnson wrote:<br class=""><br class="">> In<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D22356#487784" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D22356#487784</a>, @mehdi_amini wrote:<br class="">><br class="">> > > That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated).<br class="">> ><br class="">> ><br class="">> > How is the final link invocation computed right now?<br class="">><br class="">><br class="">> The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)<br class=""><br class=""><br class=""></span>The question is: in the presence of static archives, how do you generates --start-lib/--end-lib? This seems to already require some build-system integration?<br class=""></blockquote><div class=""><br class=""></div></span><div class="">There is one to one mapping from bitcode .o and native .o -- so the command line can be formed.</div><span class=""><div class=""> </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 class="">> ><br class=""><br class="">><br class=""><br class="">> ><br class=""><br class="">><br class=""><br class="">> > > Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.<br class=""><br class="">><br class=""><br class="">> ><br class=""><br class="">><br class=""><br class="">> ><br class=""><br class="">><br class=""><br class="">> > Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.<br class=""><br class="">><br class=""><br class="">><br class=""><br class="">> When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.<br class=""><br class=""><br class=""></span>I'm only talking about ThinLTO and the two-stage linking, i.e. the second invocation of the linker does not end-up with the same prevailing resolution as the first invocation. Your current patches are working around this deficiency.<br class=""></blockquote><div class=""><br class=""></div><div class=""><br class=""></div></span><div class="">Since they are orginally from LinkOnceODR defs, whatever definition is picked up is implementation dependent.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Right for the multiply defined ODR refs, it shouldn't matter that the second link picks a different copy (we just need to do the conservative thing as in this and my other patch to ensure we don't introduce backwards refs).</div><div class=""><br class=""></div><div class="">I do have a worry about non-ODR multiply defined case. E.g. In the example if the first copy originally chosen as prevailing is weak (non-odr), and due to the importing of strong references to that object we no longer linked in its symbols in the second link, we might end up linking with a different weak copy that has a different body - resulting in different program behavior. If that is a real concern then I suppose we must do something else to ensure that the same copy is prevailing in the second link…</div></div></div></blockquote><div><br class=""></div><div><br class=""></div><div>If we carry the proper linker-resolution to ThinLTO, there shouldn’t be multiple version of a weak symbol left during the second link (all of them will be turned into available_externally but the prevailing).</div><div><br class=""></div><div>Even abstracting this, if you have multiple definitions of a weak symbol while linking a program, I can’t see how they wouldn’t be ODR. It would at least rely on implementation-defined linker behavior (assuming the objets are selected by the linker if they are in static archives).</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><br class=""></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;"><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><span class=""><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 class="">><br class=""><br class="">><br class=""><br class="">> > If a list of `.o` on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.<br class=""><br class="">><br class=""><br class="">><br class=""><br class="">> In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D22467" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D22467</a><span class="Apple-converted-space"> </span>the importing and symbol resolution is made suitably conservative.<br class=""><br class=""><br class=""></span>I don't see any justification for --start-lib/--end-lib right now.<br class=""><br class=""></blockquote><div class=""><br class=""></div></span><div class="">Can you clarify what do you mean by 'justification' here?</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Reading through Mehdi's comments, I think he didn't realize we were already using --start-lib/--end-lib instead of regular archive libraries.</div></div></div></blockquote><div><br class=""></div><div>I clarified my comment about the expectation for the second link in Phab.</div><div>(But yes I didn’t grasp that you were originally passing these options)</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div></div></body></html>