[PATCH] D96409: [debug-info] refactor emitDwarfUnitLength

Fri Feb 19 14:28:44 PST 2021

dblaikie added a comment.

In D96409#2574186 <https://reviews.llvm.org/D96409#2574186>, @ikudrin wrote:

> (Moving the thread, as it's more suitable here)
>
> In D95998#2572951 <https://reviews.llvm.org/D95998#2572951>, @dblaikie wrote:
>
>> In D95998#2568193 <https://reviews.llvm.org/D95998#2568193>, @ikudrin wrote:
>>
>>> In D95998#2567599 <https://reviews.llvm.org/D95998#2567599>, @dblaikie wrote:
>>>
>>>> What if the function created and returned Hi rather than taking it as an argument?
>>>
>>> Hmm, I can't say I'm a vivid fan of this change. All methods of `MCStremer` instruct it to do something, i.e. it roles as just a performer. With this change, it now requests a caller to do some action. It looks like inverting customer vs. performer roles for this particular case.
>>
>> In the sense that it now requests the user emit the end label at some point?
>
> More in the sense that it limits the flexibility of calling code without any real profit. For example, the caller now cannot create the label in advance.

Any particular benefit to being able to do that, though? I think this is similar to rolling "Lo" into the function - the caller now can't reuse that label, but it doesn't seem especially important/beneficial to be able to reuse it, compared to simplifying the API.

> For instance, `Dwarf5AccelTableWriter::ContributionEnd` was initialized when the object of that class was created. Simple and solid. Now the initialization is postponed until a method of another class, `Dwarf5AccelTableWriter::Header` is called. The tracking is a bit more challenging now.

Fair - though I'm not sure it's a huge impediment. If the label weren't used in the header it'd probably be a bug - one that hide more silently with the code as-is (perhaps the header code would end up using some other label that is never defined/emitted - and then the assembler would produce some error about that invariant being violated).

>> It seems pretty tidy to me, and I'm pretty sure we follow this pattern elsewhere in the CodeGen/AsmPrinter code.
>
> Could you get an example? I can't remember anything similar.

Let's see what I can find...

  mcdwarf::emitListsTableHeaderStart (& related APIs like emitLoclistsTableHeader)
  AddressPool::emitHeader

Those are a couple I could find at a glance at least.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96409/new/

https://reviews.llvm.org/D96409