trap_cfi(9): introduce amd64 implementation
Needs ReviewPublic
Actions

Authored by mchoo on Sun, Sep 13, 6:19 PM.

Details

Reviewers

jhb
kib

Summary

LLDB dev working on Darwin debugging recommends using CFI instead of trapframe sniffer callbacks. CFI directives itself doesn't any effects in actual code, but I splitted fork_trampoline() into fork_trampoline() and fork_trampoline_kthread() as CFI cannot express conditionals like PC equals to zero.

MFC after: 2 weeks
MFC to: stable/14, stable/15
Sponsored by: FreeBSD Foundation

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76856
Build 73739: arc lint + arc unit

Event Timeline

mchoo created this revision.Sun, Sep 13, 6:19 PM

Herald added a subscriber: imp. · View Herald TranscriptSun, Sep 13, 6:19 PM

mchoo requested review of this revision.Sun, Sep 13, 6:19 PM

Harbormaster completed remote builds in B76856: Diff 186619.Sun, Sep 13, 6:19 PM

mchoo added a parent revision: D59648: trap_cfi(9): introduce arm64 implementation.Sun, Sep 13, 6:19 PM

mchoo added a child revision: D59650: trap_cfi(9): introduce arm implementation.

mchoo added a reviewer: kib.Sun, Sep 13, 6:33 PM

mchoo edited the summary of this revision. (Show Details)Sun, Sep 13, 9:07 PM

I do not quite understand what could be a subject for the review. It is standalone macro set, which I have no idea what supposed to do, and how are used.

sys/x86/include/frame.h
119	This comment is perhaps misplaced. Should it go somewhere where the 'trap frame CFI information' is applied?

@kib, I was going to create revisions for actual changes once the macros are accepted, but since I tested this on amd64 with lldb, I'm posting second part of the change here.

Harbormaster completed remote builds in B76886: Diff 186695.Mon, Sep 14, 3:58 PM

mchoo retitled this revision from trap_cfi(9): add assembly interface for amd64 to trap_cfi(9): introduce amd64 implementation.Mon, Sep 14, 4:03 PM

mchoo edited the summary of this revision. (Show Details)

Post first part as well

Harbormaster completed remote builds in B76887: Diff 186696.Mon, Sep 14, 4:03 PM

kib added inline comments.Tue, Sep 15, 5:44 AM

sys/amd64/amd64/exception.S
1341 ↗	(On Diff #186696)	ast should be handled for kthreads trampolines as well
1710 ↗	(On Diff #186696)	FRED event entry points need the annotations
sys/amd64/include/trap_cfi.h
17	Shouldn't .cfi_sections directive specified once, at the beginning of the asm source? At least, this is my impression from reading the gas documentation for it.
31	I do not see this macro as useful, it only obfuscates the reading. And the passing of '.' as 'name' is confusing.

Remove trap_cfi_end and use raw .cfi_endproc
Remove name from trap_cfi_entry
Remove .cfi_startproc from trap_cfi_entry. ENTRY() used in this patch doesn't have .cfi_startproc (the one in machine/asm.h does) but most architectures have only one ENTRY() which has the directive. For consistency between different architectures, omit it from trap_cfi_entry
.cfi_sections .eh_frame should only be emitted once (the original compiled and ran fine, but there's no need to have it under macro)
Added cfi directives to FRED code paths
kthread can return from fork_exit back to trampoline, so handle AST

Harbormaster completed remote builds in B76958: Diff 186825.Tue, Sep 15, 11:59 PM

mchoo marked 4 inline comments as done.Wed, Sep 16, 12:00 AM

How do you validate the correctness of the .cfi annotations?

sys/amd64/include/trap_cfi.h
39	Did you checked this both with clang as and binutils as? I remember I having troubles with non-gprs, in particular segment registers names. See sys/amd64/amd64/sigtramp.S. If some `#ifdef 0`'s can be enabled there, I think it is quite important for userspace debugging.
48	Might be call this macro 'trap_cfi_trapframe'?

In D59649#1371990, @kib wrote:

How do you validate the correctness of the .cfi annotations?

I ran full kernel build with default buildkernel. I haven't tested gnu toolchain (although as I noted in the reply, current version of gas can correctly interpret non-gpr names except for %rflags). This eliminates having cfi directives outside .cfi_startproc and .cfi_endproc (This happened a few times while I was modifying macros).

So far, I verified identical backtrace and register info with LLDB (CFI) and KGDB (frame sniffer) for fork_trampoline_kthread() and nmi_calltrap. It's very tedious to check validity of every CFI directive because I need to obtain a dump (or dumps) that cover panics from all the relevant locations.

Because LLM is pretty good at repetitive chores, I let it check the kernel binary and see if CFIs are marked as signal frame, directives and disassmebly match, etc. I can manually go over all the symbols if you want, but I don't think llm's hallucination likely happened here.

sys/amd64/include/trap_cfi.h
39	.text .globl cfi_segments .type cfi_segments, @function cfi_segments: .cfi_startproc simple .cfi_def_cfa %rsp, 40 .cfi_offset %cs, -32 .cfi_offset 49, -24 /* rflags */ .cfi_offset %ss, -8 ret .cfi_endproc .size cfi_segments, . - cfi_segments .section .note.GNU-stack,"",@progbits Both clang 21 and as 2.44 compile this code and emit same result (through `llvm-dwarfdump --eh-frame`). Both compiles to fail for %rflags instead of 49. I have not tested on older versions though.
48	Do you want `trap_cfi_entry state=full` to be `state=trapframe` as well?