[debug-info] [NFC] add is-a(isa<>) support for MCStreamer

[shchenz]

Summary

This is a simple refactor. In some cases, we need to check if a MCStreamer pointer is MCAsmStreamer or MCObjectStreamer, so we can implement some different behaviour in assembly mode and object mode.

For example, we are going to support dwarf for XCOFF. XCOFF assembler will fill the .debug_line or .debug_info length into the final object file, so in assembly mode, compiler should not emit .debug_line or .debug_info length into the section header, but in object mode, we still need that length to make other tools happy.

[nemanjai]

Added Eric as the owner of debug info.

[shchenz]

In D94668#2497861, @nemanjai wrote:

Added Eric as the owner of debug info.

thanks for your kindly help. @nemanjai

[echristo]

I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.

-eric

[shchenz]

In D94668#2500530, @echristo wrote:

I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.

-eric

Hi Eric,
Thanks for your review. I made some background for this patch in https://reviews.llvm.org/D94670.

On AIX, the assembler does not need the assembly file contains the dwarf sections length info in the dwarf section header(if the dwarf section has header.) Instead, the assembler will insert the calculated length into dwarf sections header of the final object according to DWARF type. Namely AIX assembler will insert 4 bytes in each section header for DWARF32 and 12 bytes for DWARF64.

So the issue is for assembly mode, we don't need the dwarf section length in the dwarf section header as assembler will insert this info. But for object mode, we still need the dwarf section length as other tool like linker will need this.

Currently the dwarf section length is emitted by base class MCStreamer, so I need to check if MCStreamer is MCAsmStreamer or not for XCOFF. If it is MCAsmStreamer, we don't emit length info; if it is MCObjectStreamer, we need to emit the length info.

XCOFF may also need to emit .debug_line info according to Streamer type. AIX assembler does not support .loc, .file directive, so we have to write another method to emit .debug_line content for MCAsmStreamer.

Do we have other ways for this kind of requirement?

[ikudrin]

Is it possible to encapsulate the target-specific logic into some derived classes from MCStreamer so that MCStreamer itself and DWARF emitter classes work through a common interface and have no target-specific adjustments?

[shchenz]

In D94668#2500791, @ikudrin wrote:

Is it possible to encapsulate the target-specific logic into some derived classes from MCStreamer so that MCStreamer itself and DWARF emitter classes work through a common interface and have no target-specific adjustments?

Thanks for your comments @ikudrin I know what you mean. But for the use cases in D95518, we need to differentiate MCAsmStreamer and MCObjectStreamer in many classes that contain an MCStreamer pointer. I am not sure if a common interface in MCStreamer class can be easily and logically found. For example the uses in file lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp in D95518. Could you please help to look at the consumer patch D95518 and tell me what you think? Thanks again.

[ikudrin]

This comment was removed by ikudrin.

[shchenz]

Gentle ping...

We still need this patch for D95518 for lib/Target/PowerPC/PPCAsmPrinter.cpp file. We need to emit the end symbol for asmstreamer to tell where is the end of the text section.

[MaskRay]

In PPCAsmPrinter.cpp, if you use getStreamerKind() == MCStreamer::StreamerKindAs, then the code motion into MCAsmStreamer.h will not be needed. The classof functions are unused, too.

[MaskRay]

isa in the subject confused me - I thought it was "An unsigned integer whose value encodes the applicable instruction set architecture for the current instruction" in the line number information, given the debug info context.

It is actually https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html

[shchenz]

Thanks for review @MaskRay

In D94668#2553332, @MaskRay wrote:

isa in the subject confused me - I thought it was "An unsigned integer whose value encodes the applicable instruction set architecture for the current instruction" in the line number information, given the debug info context.

It is actually https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html

Right, the isa in the subject is for is-a used in llvm style RTTI.

In D94668#2553331, @MaskRay wrote:

In PPCAsmPrinter.cpp, if you use getStreamerKind() == MCStreamer::StreamerKindAs, then the code motion into MCAsmStreamer.h will not be needed. The classof functions are unused, too.

I am not very clear about this comment, I think isa<MCAsmStreamer> is more common usage. We don't need to explicitly call getStreamerKind() to do a type check. For example, when we check an AllocaInst, we won't do this like I->getOpcode() == Instruction::Alloca, we do it like isa<AllocaInst>(I).

[MaskRay]

In D94668#2553371, @shchenz wrote:

Thanks for review @MaskRay

In D94668#2553332, @MaskRay wrote:

isa in the subject confused me - I thought it was "An unsigned integer whose value encodes the applicable instruction set architecture for the current instruction" in the line number information, given the debug info context.

It is actually https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html

Right, the isa in the subject is for is-a used in llvm style RTTI.

In D94668#2553331, @MaskRay wrote:

In PPCAsmPrinter.cpp, if you use getStreamerKind() == MCStreamer::StreamerKindAs, then the code motion into MCAsmStreamer.h will not be needed. The classof functions are unused, too.

I am not very clear about this comment, I think isa<MCAsmStreamer> is more common usage. We don't need to explicitly call getStreamerKind() to do a type check. For example, when we check an AllocaInst, we won't do this like I->getOpcode() == Instruction::Alloca, we do it like isa<AllocaInst>(I).

Both ->isXXXX and isa<...> are common. isa<MCAsmStreamer> requires you to move the code to a separate header. I don't think it justifies the code motion.

[ikudrin]

In D94668#2553258, @shchenz wrote:

Gentle ping...

We still need this patch for D95518 for lib/Target/PowerPC/PPCAsmPrinter.cpp file. We need to emit the end symbol for asmstreamer to tell where is the end of the text section.

That is yet another case when the patch tries to break the abstraction. It would be better to rewrite it in a more general way that avoids casting. Thus, this patch will not be needed.

[shchenz]

In D94668#2553382, @ikudrin wrote:

In D94668#2553258, @shchenz wrote:

Gentle ping...

We still need this patch for D95518 for lib/Target/PowerPC/PPCAsmPrinter.cpp file. We need to emit the end symbol for asmstreamer to tell where is the end of the text section.

That is yet another case when the patch tries to break the abstraction. It would be better to rewrite it in a more general way that avoids casting. Thus, this patch will not be needed.

Do you have any idea about how to do this? @ikudrin My requirement here is: I need to add the endSymbol for asm streamer at the end of .text section to indicate the end of the text section.
Object streamer can easily do this by calling MCSymbol *endSection(Section) in emitDwarfLineTable, but this does not work for asm streamer. When we are generating assembly output for .debug_line section, we can not go back to .text section and emit an end symbol there. I think this is not supported.

[ikudrin]

In D94668#2553464, @shchenz wrote:

Do you have any idea about how to do this? @ikudrin My requirement here is: I need to add the endSymbol for asm streamer at the end of .text section to indicate the end of the text section.
Object streamer can easily do this by calling MCSymbol *endSection(Section) in emitDwarfLineTable, but this does not work for asm streamer. When we are generating assembly output for .debug_line section, we can not go back to .text section and emit an end symbol there. I think this is not supported.

Does that mean that endSection() does not work for your target? In that case, you need to fix it first, no?

[shchenz]

In D94668#2553568, @ikudrin wrote:

In D94668#2553464, @shchenz wrote:

Do you have any idea about how to do this? @ikudrin My requirement here is: I need to add the endSymbol for asm streamer at the end of .text section to indicate the end of the text section.
Object streamer can easily do this by calling MCSymbol *endSection(Section) in emitDwarfLineTable, but this does not work for asm streamer. When we are generating assembly output for .debug_line section, we can not go back to .text section and emit an end symbol there. I think this is not supported.

Does that mean that endSection() does not work for your target? In that case, you need to fix it first, no?

This seems not like a target specific problem. It should be by design, we can not call changeSection back to an existing section when we generate assembly output?.
endSection() will call changeSection and changeSection of MCAsmStreamer will call PrintSwitchToSection if target does not create a target-specific streamer.

void MCAsmStreamer::changeSection(MCSection *Section,
                                  const MCExpr *Subsection) {
  assert(Section && "Cannot switch to a null section!");
  if (MCTargetStreamer *TS = getTargetStreamer()) {
    TS->changeSection(getCurrentSectionOnly(), Section, Subsection, OS);
  } else {
    Section->PrintSwitchToSection(
        *MAI, getContext().getObjectFileInfo()->getTargetTriple(), OS,
        Subsection);
  }
}

In PrintSwitchToSection of MCSectionXCOFF::PrintSwitchToSection, we only generate some section headers in the assembly output, so I guess we can not switch to .text section and then emit a text end there, if we do so, we get pseudo assembly code like:

.text

.dwabbrev
...
.dwline
...
.text  #switch to .text
.text_end
.dwline_end #switch back to .dwline

I guess this is not allowed? MCSectionELF::PrintSwitchToSection also has same issue.

I have made some changes in D95518, and now the cast operation is not needed anymore. So this patch can be abandoned.