[PATCH] D149367: Emit the CodeView `S_ARMSWITCHTABLE` debug symbol for jump tables
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 9 14:05:37 PDT 2023
efriedma added inline comments.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3483
+ // and the jump table that it uses in the debug info.
+ if (MO.getType() == MachineOperand::MO_JumpTableIndex) {
+ // For label-difference jump tables, find the base expression.
----------------
I'm concerned this could fail to find the jump table, or find the wrong jump table. It is possible to hoist some jump-table-related operations into an earlier block. For example:
```
extern "C" void f1();
extern "C" void f2();
extern "C" void f3();
extern "C" void f4();
extern "C" void f5();
extern "C" void func(int i, int j){
for (int k = 0; k < j; ++k) {
switch (i) {
case 0: f1(); break;
case 1: f2(); break;
case 2: f3(); break;
case 3: f4(); break;
}
}
}
```
On AArch64, we produce:
```
// %bb.1:
mov w19, w1
mov w20, w0
mov w21, w0
adrp x22, .LJTI0_0
add x22, x22, :lo12:.LJTI0_0
b .LBB0_4
.LBB0_2: // %sw.bb3
// in Loop: Header=BB0_4 Depth=1
bl f4
.LBB0_3: // %for.inc
// in Loop: Header=BB0_4 Depth=1
subs w19, w19, #1
b.eq .LBB0_9
.LBB0_4: // %for.body
// =>This Inner Loop Header: Depth=1
cmp w20, #3
b.hi .LBB0_3
// %bb.5: // %for.body
// in Loop: Header=BB0_4 Depth=1
adr x8, .LBB0_2
ldrb w9, [x22, x21]
add x8, x8, x9, lsl #2
br x8
```
Off the top of my head, I'm not sure how to write a testcase where it actually finds the wrong table, but I suspect it's possible.
We might need to encode the associated jump table into the MachineInstrs some other way. I'm not sure what that looks like, exactly; maybe a pseudo-instruction just before the jump?
================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3491
+ "EK_Inline, EK_Custom32, EK_GPRel32BlockAddress, and "
+ "EK_GPRel64BlockAddress should never be emitted for COFF");
+ case MachineJumpTableInfo::EK_BlockAddress:
----------------
dpaoliello wrote:
> efriedma wrote:
> > dpaoliello wrote:
> > > efriedma wrote:
> > > > dpaoliello wrote:
> > > > > efriedma wrote:
> > > > > > LLVM can target 32-bit ARM Windows.
> > > > > I wasn't seeing ARM32 hit this code before, but it looks like it was because of the assumption that the branch instruction wasn't the one to use the jump table value.
> > > > >
> > > > > I've fixed the code to handle ARM32 and added it to the tests, but there seems to be some bug where the offsets are always 0? If I run the test via `llc` then the correct labels appear in the debug info, but somewhere between `llc -filetype=obj` and `llvm-readobj` the offsets are lost...
> > > > In general, .secrel32 emits a relocation; `llvm-readobj --codeview` won't dump it, I think, but it should still be there. What's the relocation pointing to?
> > > The "Branch" and "Base" are pointing to a label for the `ADD` instruction, and "Table" is pointing to the Jump Table itself:
> > >
> > > If I run `llc -mtriple=thumbv7a-windows < llvm\test\DebugInfo\COFF\jump-table.ll"` I get:
> > >
> > > ```
> > > .text
> > > .syntax unified
> > > .file "jump-table.cpp"
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > '+cx8' is not a recognized feature for this target (ignoring feature)
> > > '+fxsr' is not a recognized feature for this target (ignoring feature)
> > > '+mmx' is not a recognized feature for this target (ignoring feature)
> > > '+sse' is not a recognized feature for this target (ignoring feature)
> > > '+sse2' is not a recognized feature for this target (ignoring feature)
> > > '+x87' is not a recognized feature for this target (ignoring feature)
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > '+cx8' is not a recognized feature for this target (ignoring feature)
> > > '+fxsr' is not a recognized feature for this target (ignoring feature)
> > > '+mmx' is not a recognized feature for this target (ignoring feature)
> > > '+sse' is not a recognized feature for this target (ignoring feature)
> > > '+sse2' is not a recognized feature for this target (ignoring feature)
> > > '+x87' is not a recognized feature for this target (ignoring feature)
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > 'x86-64' is not a recognized processor for this target (ignoring processor)
> > > .def func;
> > > .scl 2;
> > > .type 32;
> > > .endef
> > > .globl func @ -- Begin function func
> > > .p2align 2
> > > .code16 @ @func
> > > .thumb_func
> > > func:
> > > $Mfunc_begin0:
> > > .cv_func_id 0
> > > .cv_file 1 "C:\\llvm\\jump-table.cpp" "35610C7104C8080F83E2BF6A02DABFC9" 1
> > > .cv_loc 0 1 6 0 @ .\jump-table.cpp:6:0
> > > @ %bb.0:
> > > push {r7, lr}
> > > sub sp, #8
> > > str r0, [sp, #4]
> > > $Mtmp0:
> > > .cv_loc 0 1 7 0 @ .\jump-table.cpp:7:0
> > > ldr r0, [sp, #4]
> > > cmp r0, #3
> > > bhi ($MBB0_7)
> > > @ %bb.1:
> > > $Mtmp1:
> > > .p2align 2
> > > add r0, pc
> > > ldrb r0, [r0, #4]
> > > lsls r0, r0, #1
> > > .LCPI0_0:
> > > add pc, r0
> > > @ %bb.2:
> > > .p2align 2
> > > .LJTI0_0:
> > > .byte (($MBB0_3)-(.LCPI0_0+4))/2
> > > .byte (($MBB0_4)-(.LCPI0_0+4))/2
> > > .byte (($MBB0_5)-(.LCPI0_0+4))/2
> > > .byte (($MBB0_6)-(.LCPI0_0+4))/2
> > > .p2align 1
> > > $MBB0_3:
> > > $Mtmp2:
> > > .cv_loc 0 1 8 0 @ .\jump-table.cpp:8:0
> > > bl f1
> > > b ($MBB0_7)
> > > $MBB0_4:
> > > .cv_loc 0 1 9 0 @ .\jump-table.cpp:9:0
> > > bl f2
> > > b ($MBB0_7)
> > > $MBB0_5:
> > > .cv_loc 0 1 10 0 @ .\jump-table.cpp:10:0
> > > bl f3
> > > b ($MBB0_7)
> > > $MBB0_6:
> > > .cv_loc 0 1 11 0 @ .\jump-table.cpp:11:0
> > > bl f4
> > > b ($MBB0_7)
> > > $Mtmp3:
> > > $MBB0_7:
> > > .cv_loc 0 1 13 0 @ .\jump-table.cpp:13:0
> > > ldr r0, [sp, #4]
> > > subs r0, r0, #1
> > > cmp r0, #4
> > > bhi ($MBB0_15)
> > > @ %bb.8:
> > > $Mtmp4:
> > > .p2align 2
> > > add r0, pc
> > > ldrb r0, [r0, #4]
> > > lsls r0, r0, #1
> > > .LCPI0_1:
> > > add pc, r0
> > > @ %bb.9:
> > > .p2align 2
> > > .LJTI0_1:
> > > .byte (($MBB0_10)-(.LCPI0_1+4))/2
> > > .byte (($MBB0_11)-(.LCPI0_1+4))/2
> > > .byte (($MBB0_12)-(.LCPI0_1+4))/2
> > > .byte (($MBB0_13)-(.LCPI0_1+4))/2
> > > .byte (($MBB0_14)-(.LCPI0_1+4))/2
> > > .p2align 1
> > > $MBB0_10:
> > > $Mtmp5:
> > > .cv_loc 0 1 14 0 @ .\jump-table.cpp:14:0
> > > bl f2
> > > b ($MBB0_15)
> > > $MBB0_11:
> > > .cv_loc 0 1 15 0 @ .\jump-table.cpp:15:0
> > > bl f3
> > > b ($MBB0_15)
> > > $MBB0_12:
> > > .cv_loc 0 1 16 0 @ .\jump-table.cpp:16:0
> > > bl f4
> > > b ($MBB0_15)
> > > $MBB0_13:
> > > .cv_loc 0 1 17 0 @ .\jump-table.cpp:17:0
> > > bl f5
> > > b ($MBB0_15)
> > > $MBB0_14:
> > > .cv_loc 0 1 18 0 @ .\jump-table.cpp:18:0
> > > bl f1
> > > b ($MBB0_15)
> > > $Mtmp6:
> > > $MBB0_15:
> > > .cv_loc 0 1 20 0 @ .\jump-table.cpp:20:0
> > > add sp, #8
> > > pop {r7}
> > > pop {r0}
> > > mov lr, r0
> > > bx lr
> > > $Mtmp7:
> > > $Mfunc_end0:
> > > @ -- End function
> > > .section .debug$S,"dr"
> > > .p2align 2, 0x0
> > > .long 4 @ Debug section magic
> > > .long 241
> > > .long ($Mtmp9)-($Mtmp8) @ Subsection size
> > > $Mtmp8:
> > > .short ($Mtmp11)-($Mtmp10) @ Record length
> > > $Mtmp10:
> > > .short 4353 @ Record kind: S_OBJNAME
> > > .long 0 @ Signature
> > > .byte 0 @ Object name
> > > .p2align 2, 0x0
> > > $Mtmp11:
> > > .short ($Mtmp13)-($Mtmp12) @ Record length
> > > $Mtmp12:
> > > .short 4412 @ Record kind: S_COMPILE3
> > > .long 16385 @ Flags and language
> > > .short 244 @ CPUType
> > > .short 15 @ Frontend version
> > > .short 0
> > > .short 1
> > > .short 0
> > > .short 17000 @ Backend version
> > > .short 0
> > > .short 0
> > > .short 0
> > > .asciz "clang version 15.0.1" @ Null-terminated compiler version string
> > > .p2align 2, 0x0
> > > $Mtmp13:
> > > $Mtmp9:
> > > .p2align 2, 0x0
> > > .long 241 @ Symbol subsection for func
> > > .long ($Mtmp15)-($Mtmp14) @ Subsection size
> > > $Mtmp14:
> > > .short ($Mtmp17)-($Mtmp16) @ Record length
> > > $Mtmp16:
> > > .short 4423 @ Record kind: S_GPROC32_ID
> > > .long 0 @ PtrParent
> > > .long 0 @ PtrEnd
> > > .long 0 @ PtrNext
> > > .long ($Mfunc_end0)-func @ Code size
> > > .long 0 @ Offset after prologue
> > > .long 0 @ Offset before epilogue
> > > .long 4098 @ Function type index
> > > .secrel32 func @ Function section relative address
> > > .secidx func @ Function section index
> > > .byte 0 @ Flags
> > > .asciz "func" @ Function name
> > > .p2align 2, 0x0
> > > $Mtmp17:
> > > .short ($Mtmp19)-($Mtmp18) @ Record length
> > > $Mtmp18:
> > > .short 4114 @ Record kind: S_FRAMEPROC
> > > .long 16 @ FrameSize
> > > .long 0 @ Padding
> > > .long 0 @ Offset of padding
> > > .long 0 @ Bytes of callee saved registers
> > > .long 0 @ Exception handler offset
> > > .short 0 @ Exception handler section
> > > .long 90112 @ Flags (defines frame register)
> > > .p2align 2, 0x0
> > > $Mtmp19:
> > > .short ($Mtmp21)-($Mtmp20) @ Record length
> > > $Mtmp20:
> > > .short 4414 @ Record kind: S_LOCAL
> > > .long 116 @ TypeIndex
> > > .short 1 @ Flags
> > > .asciz "i"
> > > .p2align 2, 0x0
> > > $Mtmp21:
> > > .cv_def_range $Mtmp0 $Mtmp7, reg_rel, 23, 0, 4
> > > .short ($Mtmp23)-($Mtmp22) @ Record length
> > > $Mtmp22:
> > > .short 4441 @ Record kind: S_ARMSWITCHTABLE
> > > .secrel32 .LCPI0_0 @ Base offset
> > > .secidx .LCPI0_0 @ Base section index
> > > .short 7 @ Switch type
> > > .secrel32 .LCPI0_0 @ Branch offset
> > > .secrel32 .LJTI0_0 @ Table offset
> > > .secidx .LCPI0_0 @ Branch section index
> > > .secidx .LJTI0_0 @ Table section index
> > > .long 4 @ Entries count
> > > .p2align 2, 0x0
> > > $Mtmp23:
> > > .short ($Mtmp25)-($Mtmp24) @ Record length
> > > $Mtmp24:
> > > .short 4441 @ Record kind: S_ARMSWITCHTABLE
> > > .secrel32 .LCPI0_1 @ Base offset
> > > .secidx .LCPI0_1 @ Base section index
> > > .short 7 @ Switch type
> > > .secrel32 .LCPI0_1 @ Branch offset
> > > .secrel32 .LJTI0_1 @ Table offset
> > > .secidx .LCPI0_1 @ Branch section index
> > > .secidx .LJTI0_1 @ Table section index
> > > .long 5 @ Entries count
> > > .p2align 2, 0x0
> > > $Mtmp25:
> > > .short 2 @ Record length
> > > .short 4431 @ Record kind: S_PROC_ID_END
> > > $Mtmp15:
> > > .p2align 2, 0x0
> > > .cv_linetable 0, func, $Mfunc_end0
> > > .cv_filechecksums @ File index to string table offset subsection
> > > .cv_stringtable @ String table
> > > .long 241
> > > .long ($Mtmp27)-($Mtmp26) @ Subsection size
> > > $Mtmp26:
> > > .short ($Mtmp29)-($Mtmp28) @ Record length
> > > $Mtmp28:
> > > .short 4428 @ Record kind: S_BUILDINFO
> > > .long 4102 @ LF_BUILDINFO index
> > > .p2align 2, 0x0
> > > $Mtmp29:
> > > $Mtmp27:
> > > .p2align 2, 0x0
> > > .section .debug$T,"dr"
> > > .p2align 2, 0x0
> > > .long 4 @ Debug section magic
> > > @ ArgList (0x1000)
> > > .short 0xa @ Record length
> > > .short 0x1201 @ Record kind: LF_ARGLIST
> > > .long 0x1 @ NumArgs
> > > .long 0x74 @ Argument: int
> > > @ Procedure (0x1001)
> > > .short 0xe @ Record length
> > > .short 0x1008 @ Record kind: LF_PROCEDURE
> > > .long 0x3 @ ReturnType: void
> > > .byte 0x0 @ CallingConvention: NearC
> > > .byte 0x0 @ FunctionOptions
> > > .short 0x1 @ NumParameters
> > > .long 0x1000 @ ArgListType: (int)
> > > @ FuncId (0x1002)
> > > .short 0x12 @ Record length
> > > .short 0x1601 @ Record kind: LF_FUNC_ID
> > > .long 0x0 @ ParentScope
> > > .long 0x1001 @ FunctionType: void (int)
> > > .asciz "func" @ Name
> > > .byte 243
> > > .byte 242
> > > .byte 241
> > > @ StringId (0x1003)
> > > .short 0xe @ Record length
> > > .short 0x1605 @ Record kind: LF_STRING_ID
> > > .long 0x0 @ Id
> > > .asciz "C:\\llvm" @ StringData
> > > @ StringId (0x1004)
> > > .short 0x16 @ Record length
> > > .short 0x1605 @ Record kind: LF_STRING_ID
> > > .long 0x0 @ Id
> > > .asciz "jump-table.cpp" @ StringData
> > > .byte 241
> > > @ StringId (0x1005)
> > > .short 0xa @ Record length
> > > .short 0x1605 @ Record kind: LF_STRING_ID
> > > .long 0x0 @ Id
> > > .byte 0 @ StringData
> > > .byte 243
> > > .byte 242
> > > .byte 241
> > > @ BuildInfo (0x1006)
> > > .short 0x1a @ Record length
> > > .short 0x1603 @ Record kind: LF_BUILDINFO
> > > .short 0x5 @ NumArgs
> > > .long 0x1003 @ Argument: C:\llvm
> > > .long 0x0 @ Argument
> > > .long 0x1004 @ Argument: jump-table.cpp
> > > .long 0x1005 @ Argument
> > > .long 0x0 @ Argument
> > > .byte 242
> > > .byte 241
> > > ```
> > That makes sense... I think it's pointing to the right location, there just isn't any offset because there's a symbol at that address. (Arguably there shouldn't be such a symbol, but that's not really related to your patch.) On other targets, the closest symbol is probably the function entry point.
> >
> > Maybe we could teach llvm-readobj to dump this in a more useful way; instead of just dumping the offset, it could also dump any related relocations. But it doesn't look like the compiler is doing anything wrong.
> @efriedma I'd prefer if we didn't block this change because of the ARM32 issue: LLVM has never fully supported ARM32 Windows (for example exception unwinding isn't implemented) and from a Microsoft perspective we haven't shipped an ARM32 OS since Windows 10 IoT Core in 2018 and ARM32 support is being removed from Windows 11 (https://learn.microsoft.com/en-us/windows/arm/arm32-to-arm64)
My point was, if I'm understanding correctly, we're actually generating the correct code; it just looks weird in the dumps. So I don't think we need to do anything... except maybe add a few CHECK lines for the relocations in the regression tests. Teaching the dumping tools to make the dumps look less weird was just a "would be nice" thought.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149367/new/
https://reviews.llvm.org/D149367
More information about the llvm-commits
mailing list