arm: Unbreak debugging programs that use FP instructions
ClosedPublic
Actions

Authored by kd on Feb 20 2023, 3:02 PM.

Details

Reviewers

mw
wma
andrew
markmi_dsl-only.net

Commits

rG4d2427f2c445: arm: Unbreak debugging programs that use FP instructions

Summary

Contrary to arm64, on armv7 get_vfpcontext/set_vfpcontext can be called from cpu_ptrace.
This can be triggered when gdb hits a breakpoint in a userspace program.
Relax td == currthread assertion to account for that situation.
While here improve the copying of VFP context.
Instead of copying fpscr in a separate assignment instruction, extend memcopy to cover it.

Reported by: Mark Millard <marklmi@yahoo.com>

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kd requested review of this revision.Feb 20 2023, 3:02 PM

kd created this revision.

Do not take the following as indicating anything is necessarily wrong. It is more about my ignorance in the subjects involved.

Why does get_vfpcontext only need critical_enter/critical_exit to span so little but set_vfpcontext to span so much?

An implication is that no variation in context from the likes of, say, a cpu migration would mess up an already partially completed memcpy in get_vfpcontext. So the pcb_vfpstate storage used would apparently be unchanged over the whole memcpy operation.

With the savectx blne -> bl change, D38696.diff, and D38698.diff all applied, all
the activities with all 3 of my small example programs for the armv7 floating
point related problems look to be working just fine: no KASSERT's ( simple_dbl.c
and dbl_and_ull_via_async.cpp activities) and no odd values showing up in a
thread ( dbl_and_ull_multithread.cpp runs for minutes at a time).

markmi_dsl-only.net mentioned this in D38698: arm: Fix initialization of VFP context.Feb 20 2023, 11:15 PM

andrew added inline comments.Feb 22 2023, 12:43 PM

sys/arm/arm/exec_machdep.c
134	Why do you need to expand the critical section?

In D38696#880545, @markmi_dsl-only.net wrote:

Do not take the following as indicating anything is necessarily wrong. It is more about my ignorance in the subjects involved.

Why does get_vfpcontext only need critical_enter/critical_exit to span so little but set_vfpcontext to span so much?

An implication is that no variation in context from the likes of, say, a cpu migration would mess up an already partially completed memcpy in get_vfpcontext. So the pcb_vfpstate storage used would apparently be unchanged over the whole memcpy operation.

Sorry, I've missed your comment.
As mentioned inline the critical section was a bit too wide.
I've fixed that in the latest version of this patch.

sys/arm/arm/exec_machdep.c
134	I suppose that there is no need to do that. I just wanted to be safe, but after examinng the code I've changed this to only have vfp_discard inside the critical section.

Retesting my 3 armv7 tests based on use of the updates https://reviews.freebsd.org/D38696?id=117780 and https://reviews.freebsd.org/D38698?id=117779 worked fine.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 23 2023, 4:51 PM

Closed by commit rG4d2427f2c445: arm: Unbreak debugging programs that use FP instructions (authored by kd). · Explain Why

This revision was automatically updated to reflect the committed changes.

kd added a commit: rG4d2427f2c445: arm: Unbreak debugging programs that use FP instructions.

Herald added a subscriber: imp. · View Herald TranscriptFeb 23 2023, 4:51 PM

I did the 3 tests via booting an installed FreeBSD-14.0-CURRENT-arm-armv7-GENERICSD-20230302-005cca8361a4-261233.img.xz (from today) and they all worked fine.