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

[shchenz]

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.

[ikudrin]

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.

[shchenz]

In D95998#2550999, @ikudrin wrote:

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]

1: use emitDwarfUnitLength for debug line too

[shchenz]

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.

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

[shchenz]

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

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?

[ikudrin]

In D95998#2553440, @shchenz wrote:

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

[shchenz]

1: split refactor of emitDwarfUnitLength to patch D96409

[shchenz]

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.

[ikudrin]

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]

1: address @ikudrin comments

[shchenz]

In D95998#2563430, @ikudrin wrote:

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

[dblaikie]

In D95998#2553505, @ikudrin wrote:

In D95998#2553440, @shchenz wrote:

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

[shchenz]

In D95998#2566302, @dblaikie wrote:

In D95998#2553505, @ikudrin wrote:

In D95998#2553440, @shchenz wrote:

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

[ikudrin]

llvm/lib/MC/MCDwarf.cpp

498–501

What about using just emitAbsoluteSymbolDiff() here?

[dblaikie]

In D95998#2567407, @shchenz wrote:

In D95998#2566302, @dblaikie wrote:

In D95998#2553505, @ikudrin wrote:

In D95998#2553440, @shchenz wrote:

In 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?

[shchenz]

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]

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

[ikudrin]

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

[shchenz]

In D95998#2568193, @ikudrin wrote:

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

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!

[dblaikie]

In D95998#2568193, @ikudrin wrote:

In 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?

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]

1: update due to change in parent D96409

[dblaikie]

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.

[ikudrin]

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]

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]

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.

llvm/lib/MC/MCDwarf.cpp

MCDwarfLineTableHeader::Emit(MCStreamer *MCOS, MCDwarfLineTableParams Params,
 
   // Create a symbol at the beginning of the line table.
   MCSymbol *LineStartSym = Label;
   if (!LineStartSym)
     LineStartSym = context.createTempSymbol();
   // Set the value of the symbol, as we are at the start of the line table.
   MCOS->emitLabel(LineStartSym);
 
-  // Create a symbol for the end of the section (to be set when we get there).
-  MCSymbol *LineEndSym = context.createTempSymbol();
-
-  unsigned UnitLengthBytes =
-      dwarf::getUnitLengthFieldByteSize(context.getDwarfFormat());
   unsigned OffsetSize = dwarf::getDwarfOffsetByteSize(context.getDwarfFormat());
 
-  if (context.getDwarfFormat() == dwarf::DWARF64)
-    // Emit DWARF64 mark.
-    MCOS->emitInt32(dwarf::DW_LENGTH_DWARF64);
-
-  // The length field does not include itself and, in case of the 64-bit DWARF
-  // format, the DWARF64 mark.
-  emitAbsValue(*MCOS,
-               makeEndMinusStartExpr(context, *LineStartSym, *LineEndSym,
-                                     UnitLengthBytes),
-               OffsetSize);
+  MCSymbol *LineEndSym = MCOS->emitDwarfUnitLength("debug_line", "unit length");
 
   // Next 2 bytes is the Version.
   unsigned LineTableVersion = context.getDwarfVersion();
   MCOS->emitInt16(LineTableVersion);
 
-  // Keep track of the bytes between the very start and where the header length
-  // comes out.
-  unsigned PreHeaderLengthBytes = UnitLengthBytes + 2;
-
   // In v5, we get address info next.
   if (LineTableVersion >= 5) {
     MCOS->emitInt8(context.getAsmInfo()->getCodePointerSize());
     MCOS->emitInt8(0); // Segment selector; same as EmitGenDwarfAranges.
-    PreHeaderLengthBytes += 2;
   }
 
+  MCSymbol *ProStartSym = context.createTempSymbol();
+
   // Create a symbol for the end of the prologue (to be set when we get there).
   MCSymbol *ProEndSym = context.createTempSymbol(); // Lprologue_end
 
   // Length of the prologue, is the next 4 bytes (8 bytes for DWARF64). This is
   // actually the length from after the length word, to the end of the prologue.
-  emitAbsValue(*MCOS,
-               makeEndMinusStartExpr(context, *LineStartSym, *ProEndSym,
-                                     (PreHeaderLengthBytes + OffsetSize)),
-               OffsetSize);
+  MCOS->emitAbsoluteSymbolDiff(ProEndSym, ProStartSym, OffsetSize);
+
+  MCOS->emitLabel(ProStartSym);
 
   // Parameters of the state machine, are next.
   MCOS->emitInt8(context.getAsmInfo()->getMinInstAlignment());
   // maximum_operations_per_instruction
   // For non-VLIW architectures this field is always 1.
   // FIXME: VLIW architectures need to update this field accordingly.
   if (LineTableVersion >= 4)
     MCOS->emitInt8(1);
   MCOS->emitInt8(DWARF2_LINE_DEFAULT_IS_STMT);