Page MenuHomePhabricator

make the AsmPrinterHandler array public
ClosedPublic

Authored by vtjnash on Oct 16 2020, 10:04 PM.

Details

Summary

This lets external consumers customize the output, similar to how
AssemblyAnnotationWriter lets the caller define callbacks when printing
IR. The array of handlers already existed, this just cleans up the code
so that it can be exposed publically.

Replaces https://reviews.llvm.org/D74158

Diff Detail

Event Timeline

vtjnash created this revision.Oct 16 2020, 10:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2020, 10:04 PM
vtjnash requested review of this revision.Oct 16 2020, 10:04 PM
vtjnash edited the summary of this revision. (Show Details)Oct 16 2020, 10:05 PM
vtjnash added a reviewer: rnk.

In my first attempt, I missed that it's intentional we're claiming MMI->hasDebugInfo() if there's information available for DWARF output if using fast-isel, even if we don't need that DebugInfo because the output for it is disabled (CodeView information is still somewhat separate, as my attempt to bring them back in line is what failed the CI testing). Fortunately, that's what tests are for when this change was added in 8763c0c5b7a566d2fe9476259aa346640b0a9deb (https://reviews.llvm.org/D53885). I think this should pass all tests now, as I've now fully removed that part of the change (it passes locally anyways).

Is it okay if I attempt to re-land this now?

rnk added a comment.Oct 22 2020, 3:28 PM

Yes, I think it's OK to reland if you think you understand the previous failures and believe you've addressed them. Maybe you already did, I'm reading old email...

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Nov 3, 7:03 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
uabelho added a subscriber: uabelho.Wed, Nov 4, 4:44 AM

Hi!

Is this commit supposed to change the output from llc?

With

llc -disable-debug-info-print=true -o - unwind-tables.opt.ll

on

define i16 @main() nounwind {
entry:
  ret i16 0
}

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "unwind-tables.c", directory: "/tmp")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 12.0.0"}

I now get

.cfi_startproc
and
.cfi_endproc

in the output which I didn't before.

It seems like the behavior of -disable-debug-info-print=true has changed. Before -disable-debug-info-print=true would call MMI->setDebugInfoAvailability(false); in DwarfDebug::beginModule(), but that doesn't seem to happen anymore.

Is this expected?

The .cfi_startproc directive is part of the unwind information (eh_frame), and is not debug info, so that seems like a possible bugfix even? I'd expect that observation to be consistent with the goal of this PR, though I didn't see specifically where that pass was being affected in a cursory glance back through the PR.

The line of code you specifically mentioned was only added relatively recently, in 2018 (https://reviews.llvm.org/D53885), and made that behavior a bit odd. (The change causes FastISel to emit debug information if any DWARF information is present, even if there will not be a consumer of that information with no AsmPrinter in the pipeline)

dstenb added a subscriber: dstenb.Thu, Nov 5, 7:01 AM
dstenb added a comment.Thu, Nov 5, 9:40 AM

The .cfi_startproc directive is part of the unwind information (eh_frame), and is not debug info, so that seems like a possible bugfix even? I'd expect that observation to be consistent with the goal of this PR, though I didn't see specifically where that pass was being affected in a cursory glance back through the PR.

But the .cfi_startproc directive may not only be for eh_frame, but can also be used for only emitting DWARF unwind information?

In a case like this:

int main() {
  return 0;
}
clang --target=ppc main.c -S -gline-tables-only -mllvm -disable-debug-info-print=true

We will now emit CFI directives that only relevant for debug information:

 main:                                   # @main
 .Lfunc_begin0:
+       .file   1 "/repo/edasten/llvm-project/llvm" "main.c"
+       .loc    1 1 0                           # main.c:1:0
+       .cfi_sections .debug_frame
+       .cfi_startproc
 # %bb.0:                                # %entry
        stwu 1, -16(1)
        stw 31, 12(1)
+       .cfi_def_cfa_offset 16
+       .cfi_offset r31, -4
        mr      31, 1
+       .cfi_def_cfa_register r31
        li 3, 0
        stw 3, 8(31)
+.Ltmp0:
+       .loc    1 2 3 prologue_end              # main.c:2:3
        lwz 31, 12(1)
        addi 1, 1, 16
        blr
+.Ltmp1:
 .Lfunc_end0:
        .size   main, .Lfunc_end0-.Lfunc_begin0
+       .cfi_endproc
                                         # -- End function
        .ident  "clang version 12.0.0"
        .section        ".note.GNU-stack","",@progbits
        .addrsig
+       .section        .debug_line,"",@progbits
+.Lline_table_start0:

Also .file and .loc directives, and a .debug_line section are emitted. I think that this should be considered as breaking the (although hidden) -disable-debug-info-print flag.

The line of code you specifically mentioned was only added relatively recently, in 2018 (https://reviews.llvm.org/D53885), and made that behavior a bit odd. (The change causes FastISel to emit debug information if any DWARF information is present, even if there will not be a consumer of that information with no AsmPrinter in the pipeline)

Ah, okay, yes that is more clearly needing to be fixed. I think I just need to rearrange how that command line option works.