This is an archive of the discontinued LLVM Phabricator instance.

[debug-info] refactor emitDwarfUnitLength
ClosedPublic

Authored by shchenz on Feb 10 2021, 4:41 AM.

Details

Summary

This is from code review comments in D95998, see https://reviews.llvm.org/D95998#2553505

comments from @ikudrin

The idea was to remove the Lo argument from MCStreamer::emitDwarfUnitLength(Hi, Lo, Comment) so that it becomes just MCStreamer::emitDwarfUnitLength(Hi, Comment).

Diff Detail

Event Timeline

shchenz created this revision.Feb 10 2021, 4:41 AM
shchenz requested review of this revision.Feb 10 2021, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 4:41 AM
MaskRay accepted this revision.Feb 14 2021, 10:11 AM
This revision is now accepted and ready to land.Feb 14 2021, 10:11 AM
ikudrin added inline comments.Feb 15 2021, 1:51 AM
llvm/lib/MC/MCStreamer.cpp
1013

The line contains trailing space. Please, remove.

llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
330

Lo is no longer used in the tests and should be removed.

332

Sec should be local to the init method.

346

I'd prefer to check that a label passed as the second argument is emitted after calling emitAbsoluteSymbolDiff(), but that requires many changes not directly connected with the purpose of this patch, so that should be done separately. I'll prepare the patch a bit later.

@shchenz, please take a look at D96710. When it is accepted, you can integrate it here or commit it separately within the whole set.

shchenz updated this revision to Diff 323747.Feb 15 2021, 7:33 AM

1: address comments

shchenz added a comment.EditedFeb 15 2021, 7:34 AM

@MaskRay @ikudrin Thanks for your review. Also thanks for your help with the test case changes, Igor.

ikudrin added inline comments.Feb 16 2021, 7:44 AM
llvm/test/DebugInfo/X86/addr_comments.ll
4–6

It is better to avoid checking specific names of temporary labels. They may change with changes in other parts, which makes the test fragile. What about using substitutions instead?

llvm/test/DebugInfo/X86/dwarf-pubnames-split.ll
10

This label now does not identify the section well and also makes the test fragile. How about this:

; CHECK:      .section .debug_pubtypes
; CHECK-NEXT: .long
; CHECK-NEXT: .Ltmp{{.+}}:
...
llvm/test/DebugInfo/X86/length_symbol_difference.ll
3–5

Ditto.

llvm/test/MC/WebAssembly/comdat-sections.ll
20–22

The same.

shchenz updated this revision to Diff 324179.Feb 16 2021, 8:57 PM

1: use RE to represent the length label

@ikudrin Thanks for your comment. I guess LBL is for label length, so I also used it for .debug_info section.

shchenz updated this revision to Diff 324219.Feb 17 2021, 12:55 AM
shchenz marked 2 inline comments as done.
shchenz added a subscriber: dblaikie.

1: address @dblaikie comments in D95998

shchenz updated this revision to Diff 324501.Feb 17 2021, 7:29 PM

1: integrate D96710

dblaikie accepted this revision.Feb 18 2021, 1:36 PM

Generally looks good to me (please wait for @ikudrin too, to ensure their issues have been addressed)

At some point it'd probably be good to include a second (probably as the first parameter) to this function to use as a prefix for the label names to make them more informative/easier to read/modify/etc. And the function may need to be renamed - it currently includes the word "unit" un it, even though it can be/is used for other DWARF sections, not just the debug_info/debug_types unit sections.

(Moving the thread, as it's more suitable here)

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. 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.

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.

At some point it'd probably be good to include a second (probably as the first parameter) to this function to use as a prefix for the label names to make them more informative/easier to read/modify/etc.

Maybe it'll make sense to do that in this patch. There will be fewer changes in the tests.

And the function may need to be renamed - it currently includes the word "unit" un it, even though it can be/is used for other DWARF sections, not just the debug_info/debug_types unit sections.

The method emits a unit length field. The name is used throughout the DWARF standard for all kinds of tables. And it hardly can be used for anything else because it may emit a DWARF64 mark.

(Moving the thread, as it's more suitable here)

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.

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.

Is std::pair<MCSymbol *, MCSymbol *>MCDwarfLineTableHeader::Emit( also an example for this? It returns LineEndSym to its caller and its caller will emits the label.

At some point it'd probably be good to include a second (probably as the first parameter) to this function to use as a prefix for the label names to make them more informative/easier to read/modify/etc.

Maybe it'll make sense to do that in this patch. There will be fewer changes in the tests.

Sure, I will update this when the API design is settled.

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.

Why should a service class like MCStreamer define the architecture of its clients? It is just not its area of responsibility.

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).

I agree that the difference is subtle. If I am not able to convince you, well, so be it. Not a big deal. Just a nice theoretical prattling.

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.

Yeah, they seem similar but aren't they merely helper methods for a limited number of well-known callers? AddressPool::emitHeader is even private. They are not methods in a distant service class, where callers are not known in advance.

Is std::pair<MCSymbol *, MCSymbol *>MCDwarfLineTableHeader::Emit( also an example for this? It returns LineEndSym to its caller and its caller will emits the label.

Note that MCDwarfLineTableHeader is a private class of MCDwarfLineTable. So this is also an implementation peculiarity of MCDwarfLineTable which does not affect others.

shchenz updated this revision to Diff 325154.Feb 19 2021, 9:55 PM

1: add symbol prefix as a parameter of emitDwarfUnitLength

Thanks for your review @ikudrin @dblaikie Both of you provide good thoughts about how to design a fundamental API. Let's design emitDwarfUnitLength like this for now. If we find some obvious weakness in the future, I think it is not hard for us to change to another design.

dblaikie accepted this revision.Feb 20 2021, 9:34 AM

Seems good to me. Wouldn't mind second set of eyes especially on the unit test - the mock stuff I'm not especially familiar with.

@dblaikie thanks for your review, David.
@ikudrin could you please help to have another look, Igor? Do you have any objections if I start to commit the patches like D95932, D95998? Thanks.

ikudrin accepted this revision.Feb 22 2021, 2:20 AM

LGTM, thanks!

llvm/lib/MC/MCStreamer.cpp
1012–1013

Maybe use "_start" and "_end" here and avoid passing the trailing '_' in the argument?

shchenz added inline comments.Feb 22 2021, 4:18 AM
llvm/lib/MC/MCStreamer.cpp
1012–1013

hmm, if Prefix is passed as "", will it be a little strange to have a label like: .L_start0?

ikudrin added inline comments.Feb 22 2021, 5:07 AM
llvm/lib/MC/MCStreamer.cpp
1012–1013

The signature of the method does not encourage a caller to pass an empty string because there is no default value for the argument. To gracefully support the empty string, it would be better not to add a suffix in that case. But we probably do not need that kind of fine-tuning right now.

shchenz added inline comments.Feb 22 2021, 5:38 AM
llvm/lib/MC/MCStreamer.cpp
1012–1013

OK, I will update this later. Thanks.

shchenz updated this revision to Diff 325645.Feb 22 2021, 7:15 PM

1: put "_" in the function body to avoid strange string parameters

This revision was automatically updated to reflect the committed changes.