This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shchenz on Feb 3 2021, 10:15 PM.

Details

Summary

This is to do the same things with D95932.
We may want to some customization for DWARF unit length in DWARF section headers for some code generation path.

This patch focuses on debug line section. We also use emitDwarfUnitLength for debug line now, so we can benefit from overriding of emitDwarfUnitLength inside different streamer too.

Diff Detail

Event Timeline

shchenz created this revision.Feb 3 2021, 10:15 PM
shchenz requested review of this revision.Feb 3 2021, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 10:15 PM

There is no need to create a new MCStreamer::emitDwarfLineUnitLength() method and make all the dependencies public. It is better to just use the already existed MCStreamer::emitDwarfUnitLength(). All you need to do is to create a temporary symbol and emit it right after calling emitDwarfUnitLength().

By the way, the said MCStreamer::emitDwarfUnitLength() can be simplified so that it is called only with the Hi argument, without Lo. All the call sites create a temporary symbol, pass it to the method, and emitted it after that. All that can be done inside the method so that the calling code will be simplified. Your other patches in the set will also benefit from that change.

There is no need to create a new MCStreamer::emitDwarfLineUnitLength() method and make all the dependencies public. It is better to just use the already existed MCStreamer::emitDwarfUnitLength(). All you need to do is to create a temporary symbol and emit it right after calling emitDwarfUnitLength().

By the way, the said MCStreamer::emitDwarfUnitLength() can be simplified so that it is called only with the Hi argument, without Lo. All the call sites create a temporary symbol, pass it to the method, and emitted it after that. All that can be done inside the method so that the calling code will be simplified. Your other patches in the set will also benefit from that change.

Thanks, I added a FIXME in MCStreamer::emitDwarfLineUnitLength() to plan to implement emitDwarfLineUnitLength in emitDwarfUnitLength.
I will try to implement this as you said.

shchenz updated this revision to Diff 322588.Feb 9 2021, 9:41 PM
shchenz retitled this revision from [Debug-Info] [NFC] move emit debug line unit length to MCStreamer class to [Debug-Info] [NFC] use emitDwarfUnitLength to handle debug line section unit length field.
shchenz edited the summary of this revision. (Show Details)

1: use emitDwarfUnitLength for debug line too

Because now, we call emitLabel inside emitDwarfUnitLength and emitLabel is not a constant function and it needs to be emitted inside a section, so we:
1: change the prototype for emitDwarfUnitLength to use non-const MCSymbol;
2: add MCSection in unit test when testing for emitDwarfUnitLength

@ikudrin Could you please help to have another look? Thanks.

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.

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.

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.

@ikudrin what do you think?

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

shchenz updated this revision to Diff 322652.Feb 10 2021, 4:41 AM

1: split refactor of emitDwarfUnitLength to patch D96409

thanks, @ikudrin , I split the refactor part of emitDwarfUnitLength to patch D96409, this patch now only focus on using emitDwarfUnitLength for debug line section. Could you please help to have another look? Thanks.

I guess we can simplify that even further by emitting the temporary symbol right after the "length of the prologue" field. In that case, PreHeaderLengthBytes will not be needed at all.

shchenz updated this revision to Diff 323748.Feb 15 2021, 7:36 AM

1: address @ikudrin comments

I guess we can simplify that even further by emitting the temporary symbol right after the "length of the prologue" field. In that case, PreHeaderLengthBytes will not be needed at all.

Very good catch. Thanks for your helpful comments for these debug info related patches. @ikudrin

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)

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.

ikudrin added inline comments.Feb 16 2021, 10:59 PM
llvm/lib/MC/MCDwarf.cpp
498–501

What about using just emitAbsoluteSymbolDiff() here?

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?

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

Yeah, that is for Lo parameter.

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

Good idea, then we have a unified behaviour for Hi and Lo label. I will update patch D96409 later.

llvm/lib/MC/MCDwarf.cpp
498–501

Yeah, good idea. When I updated the patch, I found the case in D95518 has a strange label defination (L..tmp5-L..debug_line_1)-0. -0 is meaningless. But I found there are many places in MCDwarf.cpp using makeEndMinusStartExpr(, , , 0), so I was lazy to use the same way:) and thinking we can make an improvement patch later.

I will fix this place in next update.

shchenz updated this revision to Diff 324220.Feb 17 2021, 12:58 AM

1: update due to change in D96409
2: use emitAbsoluteSymbolDiff for 0 offset

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.

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.

Hmm, a good point again. But I am a little favor of the current design. The function name is emitDwarfUnitLength, so maybe the role of the Hi and Lo symbol should be the same? If we have only one of them as the function parameter, like only having Hi, without checking the function implementation or good comments, we may be a little strange about why the length only has one bound Hi? If we pass both of them, then that's ok, only from the function prototype, we know we pass the two bounds from the caller side; if we pass none of them, that is easy to understand too, callee will treat the two bound inside.

I am ok with both solutions, I will let this decision be made by debug-info experts like you @dblaikie @ikudrin

Thanks for your review!

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?

It seems pretty tidy to me, and I'm pretty sure we follow this pattern elsewhere in the CodeGen/AsmPrinter code. "Start a length-prefixed region" and provides the label for the caller to mark the end of that region doesn't seem like a fundamentally strange or surprising API design.

shchenz updated this revision to Diff 325156.Feb 19 2021, 10:01 PM

1: update due to change in parent D96409

dblaikie accepted this revision.Feb 20 2021, 10:43 AM

Looks good to me (maybe rename the "debug_line_" symbol name, having an underscore at the end is a bit weird (but I realize "debug_line" doesn't work, and "debug_line_start" will be taken by the unit length labels - so I don't have any great ideas))

Please wait for @ikudrin to make sure his feedback is addressed too.

This revision is now accepted and ready to land.Feb 20 2021, 10:43 AM
ikudrin accepted this revision.Feb 22 2021, 2:39 AM

Looks good, thanks!

llvm/lib/MC/MCDwarf.cpp
491

Don't think a prefix for this temporary symbol is needed. Even ProEndSym, which is emitted much farther from the expression, does not have it.

And maybe rename it to something like ProStartSym to match the other one?

dblaikie added inline comments.Feb 22 2021, 11:53 AM
llvm/lib/MC/MCDwarf.cpp
491

Generally names help with editing/reading/modifying/FileChecking the assembly.

ProEndSym even has a comment that "suggests" a name but doesn't use it - I don't know why it doesn't actually name the temp symbol - it probably should.

shchenz updated this revision to Diff 325646.Feb 22 2021, 7:17 PM

1: use more meaningful variable name.

llvm/lib/MC/MCDwarf.cpp
491

Yeah, agree. We should use names for the debug line prologue. This should be out of this patch's scope. Let's do it in another follow-up patch.