<div dir="ltr"><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">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aprantl added a comment.<br>
<br>
In <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>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:0 0 0 .8ex;border-left:1px #ccc solid;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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D32648" rel="noreferrer" target="_blank">https://reviews.llvm.org/D32648</a><br>
<br>
<br>
<br>
</blockquote></div></div>