Page MenuHomeFreeBSD

x86: consolidate hw watchpoint logic into new file
ClosedPublic

Authored by mhorne on Mar 9 2021, 6:15 PM.

Details

Summary

This is a prerequisite to using these functions outside of ddb, but also
provides some cleanup and minor refactoring. This code is almost
entirely duplicated between the two architectures, the only significant
difference being the lack of dbreg synchronization on i386.

Cleanups are:

  • demote some internal functions to static
  • use the constant NDBREGS instead of a '4' literal
  • remove K&R definitions
  • some added comments
Test Plan

Compiled on amd64 and i386. Tested setting/triggering hardware watchpoints in ddb on amd64.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mhorne requested review of this revision.Mar 9 2021, 6:15 PM
sys/i386/i386/db_trace.c
792–795

Note that this block is dropped, but can be added back if desired.

sys/conf/files.x86
322

Does the content of the file needed for !DDB?

sys/x86/x86/dbreg.c
29

Was this code copy/pasted with trivial modifications?
I really do not think that the new license is applicable to it.

sys/conf/files.x86
322

No, they aren't used elsewhere yet. By the end of my patch series it can become ddb | gdb.

sys/x86/x86/dbreg.c
29

I made some new additions to this file in D29155, but still something that can be considered trivial. I do not feel strongly about keeping the notice.

Remove copyright notice, make the file optional on DDB.

sys/x86/include/reg.h
270 ↗(On Diff #85423)

My last suggestion, is to move these declarations to e.g. x86/include/x86_var.h
reg.h is more a kind of ABI interface between kernel and userspace, and existing fill/set declarations are arguably a mistake (see also the comment above them).

Move function declarations to x86_var.h.

Also move amd64_db_resume_dbreg(), as it is in the scope of the new file.

kib added inline comments.
sys/x86/x86/dbreg.c
37

Isn't this #ifdef redundant? Even if the file is used for gdb && !ddb configs eventually.

This revision is now accepted and ready to land.Mar 12 2021, 11:51 AM

Remove redundant #ifdef DDB around include. Add #ifdef DDB around functions using db_printf.

Fix calculation of wsize in dbreg_set_watchpoint on i386.

This revision now requires review to proceed.Tue, Mar 16, 5:12 PM
This revision is now accepted and ready to land.Fri, Mar 19, 6:24 PM