<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Apr 28, 2017 at 2:02 PM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><blockquote type="cite"><div>On Apr 28, 2017, at 1:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-5823482747827567172Apple-interchange-newline"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Apr 28, 2017 at 12:33 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><blockquote type="cite"><div>On Apr 28, 2017, at 11:55 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-5823482747827567172m_1769420832385119872Apple-interchange-newline"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Apr 28, 2017 at 11:17 AM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">aprantl added a comment.<br><br>In<span class="m_-5823482747827567172m_1769420832385119872Apple-converted-space"> </span><a href="https://reviews.llvm.org/D32648#740966" rel="noreferrer" target="_blank">https://reviews.llvm.org/D32648#740966</a>, @dblaikie wrote:<br><br>> I'm all for this, couple of questions though:<br>><br>> - When would the use of "distinct" on an anonymous namespace matter? Since LLVM uses cross-CU references to refer to entities in different CUs, never importing/moving entities between CUs, the anonymous namespaces shouldn't collide, should they? (anymore than named namespaces would collide when there are different entities in different CUs but both in the same named namespace - LLVM still produces two separate namespaces, one in each CU, one with each of the respective entities)?<br><br><br>I believe the `distinct`  is necessary because anonymous namespaces whose parent scope is the CU have a `null` scope. (This is what DIBuilder does to every type node to allow for type uniquing.) If the anonymous namespace are not distinct, all top-level anonymous namespaces would be uniqued. Thus two types from different CU's that are in different anonymous namespaces would both have the same scope pointing to the uniqued anonymous namespace. It is not unthinkable that this could happen to work correctly because of how DwarfDebug is processing the data, but I'd rather not rely on this.<br></blockquote><div><br>I'd rather not add code that doesn't have a pretty clear rationale. "I'm not sure it'll work" leads to weird code that's hard to understand/explain because it's not necessarily founded in any actual need. (I think it was Marshall Clow who gave a good talk about "fear based programming" at cppcon a few years back which is a perspective I have a strong affinity/appreciation for)<br><br></div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>My point was that the Metadata in itself is incorrect after merging and that is worth fixing regardless of whether the DWARF backend happens to produce the right output.</div><div><br></div><div>a.cpp</div><div>namespace { int a; }</div><div>b.cpp</div><div>namespace { int b;}</div><div><br></div><div>a.ll</div><div>!0 = !DINamespace(scope: null)</div><div>!1 = !DILocalVariable(name: "a", scope: !0)</div><div><br></div><div>b.ll</div><div>!0 = !DINamespace(scope: null)</div><div>!1 = !DILocalVariable(name: "b", scope: !0)</div><div><br></div><div>a+b.ll # after llvm-link</div><div>!0 = !DINamespace(scope: null)</div><div>!1 = !DILocalVariable(name: "a", scope: !0)</div><div>!2 = !DILocalVariable(name: "b", scope: !0)</div><div><br></div><div>In my opinion it is incorrect that "a" and "b"  share the same "scope:" field (because they don't), regardless of whether it matters for the DWARF output. What do you think?</div></div></div></blockquote><div><br>Yep - I think this is about thinking about the mechanics of all this in a different way.<br><br>The way I'm considering to think about this is rather than as the DINamespace being a unique object that represents the entity - you could picture this more the same way DIFile is used - as a thing to reduce the memory usage by deduplicating the information - not because there's some logical entity that is being represented by that node.<br><br><br>Put another way - if these nodes were distinct, and an entity (say a type) referencing an anonymous namespace was emitted into the same CU, what would that do? Perhaps ideally LLVM would end up producing two anonymous namespaces in a single CU - would that help compared to putting them in the same anonymous namespace? I'm not sure any debugger would do anything especially 'better' with this output.<br></div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div>I just verified that it is not necessary to make anonymous namespaces distinct to prevent uniquing of any of their children: All DIType descendants are either distinct (such as the tag types when their UID is empty) or safe to unique (basic types, pointers, vectors, ...). All function definitions are distinct anyway. All global variables are distinct, and associated with their containing CU via the globals list.</div><div><br></div><div>I also verified that my above example, even though there is only a single anonymous DINamespace, is compiled into two DW_TAG_namespaces, one per CU.</div><div><br></div><div>So this is really just a matter of aesthetics.</div><div>Making the nodes unique is slightly more memory efficient.</div><div><br></div><div>Having confirmed that everything still works (be design, not by accident), I guess I can be convinced to use unique nodes everywhere.</div><div>Duncan, what do you think?</div></div></blockquote><div><br>Don't mind either way in this case - feel free to pick one & write a comment about it "the memory savings probably aren't worth the extra metadata merge cost of the uniquing tables" - if that turns out not to be true some time (someone decides to make a lot of small files with an anonymous namespace in each one - still, it's O(number of files/original llvm::Modules) so hardly likely to be "a lot") someone can propose changing it.<br><br>Thanks for checking/drilling down a bit - I really appreciate having a good motivation for the design choice.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>-- adrian<br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>-- adrian</div></div></div></blockquote></div></div></div></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div>I'd love to see an example of what the distinctness of these nodes achieves. (arguably distinct nodes are cheaper on LTO merging, if I recall correctly? Is that the case/am I remembering correctly? So maybe it's worth doing for that reason, but I'm not sure - it would mean more nodes/bigger metadata, though probably not by much)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> - Which tests cover the LLVM codegen side of this change - several tests no longer look for the line/file, but do any tests ensure it is not present? (if this patch were reverted, which tests would fail (not a perfect question, since the change in the metadata format would cause parse failures - but not talking about those))<br><br>test/DebugInfo/X86/dwarf-public-names.ll<br>will fail, but not for a good reason: It hardcodes the size of the CU :-)<br>I'll add CHECK-NOTs to one of the tests!<br></blockquote><div><br>Thanks!<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br><br><a href="https://reviews.llvm.org/D32648" rel="noreferrer" target="_blank">https://reviews.llvm.org/D32648</a></blockquote></div></div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></blockquote></div></div>