[PATCH] D32648: Remove line and file from DINamespace

Fri Apr 28 14:12:24 PDT 2017

On Fri, Apr 28, 2017 at 2:02 PM Adrian Prantl <aprantl at apple.com> wrote:

> On Apr 28, 2017, at 1:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Fri, Apr 28, 2017 at 12:33 PM Adrian Prantl <aprantl at apple.com> wrote:
>
>> On Apr 28, 2017, at 11:55 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> 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)
>>
>>
>> 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.
>>
>> a.cpp
>> namespace { int a; }
>> b.cpp
>> namespace { int b;}
>>
>> a.ll
>> !0 = !DINamespace(scope: null)
>> !1 = !DILocalVariable(name: "a", scope: !0)
>>
>> b.ll
>> !0 = !DINamespace(scope: null)
>> !1 = !DILocalVariable(name: "b", scope: !0)
>>
>> a+b.ll # after llvm-link
>> !0 = !DINamespace(scope: null)
>> !1 = !DILocalVariable(name: "a", scope: !0)
>> !2 = !DILocalVariable(name: "b", scope: !0)
>>
>> 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?
>>
>
> Yep - I think this is about thinking about the mechanics of all this in a
> different way.
>
> 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.
>
>
> 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.
>
>
> 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.
>
> 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.
>
> So this is really just a matter of aesthetics.
> Making the nodes unique is slightly more memory efficient.
>
> Having confirmed that everything still works (be design, not by accident),
> I guess I can be convinced to use unique nodes everywhere.
> Duncan, what do you think?
>

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.

Thanks for checking/drilling down a bit - I really appreciate having a good
motivation for the design choice.

- Dave

>
> -- adrian
>
>
>
>>
>> -- adrian
>>
>
>> 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/f7e9402c/attachment.html>