[PATCH] D95998: [Debug-Info] [NFC] use emitDwarfUnitLength to handle debug line section unit length field

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D95998#2567407 <https://reviews.llvm.org/D95998#2567407>, @shchenz wrote:

> In D95998#2566302 <https://reviews.llvm.org/D95998#2566302>, @dblaikie wrote:
>
>> In D95998#2553505 <https://reviews.llvm.org/D95998#2553505>, @ikudrin wrote:
>>
>>> In D95998#2553440 <https://reviews.llvm.org/D95998#2553440>, @shchenz wrote:
>>>
>>>> In D95998#2553412 <https://reviews.llvm.org/D95998#2553412>, @ikudrin wrote:
>>>>
>>>>> The idea was to remove the `Lo` argument from `MCStreamer::emitDwarfUnitLength(Hi, Lo, Comment)` so that it becomes just `MCStreamer::emitDwarfUnitLength(Hi, Comment)`. A temporary symbol can be created and emitted inside the method. And that should be a separate NFC patch.
>>>>
>>>> We need to differentiate the temp symbol names for different debug sections like debug info, debug pubnames, debug line to make a more readable assembly output. That's the first reason I put the temp symbol created outside.
>>>
>>> The only usage of that symbol will be an expression just before it. The readability will not be harmed. It is also possible to pass the name as an additional argument to the function if anybody finds that useful.
>>>
>>>> Another reason is for the debug line section, we need the temp symbol to generate the debug line prologue length in the following logics of function `MCDwarfLineTableHeader::Emit`. so to avoid returning the temp symbol to its caller (it is only used for debug line, but not used by other callers of `emitDwarfUnitLength`), I use the way to create the temp symbol in the caller site and to emit the temp symbol in callee site.
>>>
>>> Returning a value is usually more preferable to changeable arguments; callers may just ignore it if they do not need it. But for this case, I would just create another temporary symbol in `MCDwarfLineTableHeader::Emit()`.
>>
>> Is this feedback still outstanding? I tend to agree with it, that having emitDwarfUnitLength return the temporary MCSymbol would be a nice(r) API design. I think we do that in other parts of DWARF emission for length encodings. (up in CodeGen/AsmPrinter DWARF code)
>
> Thanks for review @dblaikie . Yes, the comment should be addressed in the new patch. The reason why we don't return the temporary `MCSymbol` for `emitDwarfUnitLength` is we don't have any caller using the returned temporary symbol. The debug_line section now uses a new locally created temporary symbol which is also a good suggestion from Igor.

Oh, I see, that comment was about using an out parameter for "Lo"?

What if the function created and returned Hi rather than taking it as an argument?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95998