[PATCH] D32648: Remove line and file from DINamespace

[David Blaikie via llvm-commits]

On Fri, Apr 28, 2017 at 11:17 AM Adrian Prantl via Phabricator <
reviews at reviews.llvm.org> wrote:

> aprantl added a comment.
>
> In https://reviews.llvm.org/D32648#740966, @dblaikie wrote:
>
> > I'm all for this, couple of questions though:
> >
> > - 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)?
>
>
> 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.
>

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)

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)


> > - 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))
>
> test/DebugInfo/X86/dwarf-public-names.ll
> will fail, but not for a good reason: It hardcodes the size of the CU :-)
> I'll add CHECK-NOTs to one of the tests!
>

Thanks!


>
>
> https://reviews.llvm.org/D32648
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170428/c7e29a8b/attachment.html>