Details

Reviewers

kib
jhb

Commits

rGc02c04f113fe: x86: consolidate hw watchpoint logic into new file

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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 37779
Build 34668: arc lint + arc unit

Event Timeline

mhorne created this revision.Mar 9 2021, 6:15 PM

Herald added subscribers: rpokala, imp. · View Herald TranscriptMar 9 2021, 6:15 PM

Harbormaster completed remote builds in B37700: Diff 85408.Mar 9 2021, 6:15 PM

mhorne requested review of this revision.Mar 9 2021, 6:15 PM

mhorne added inline comments.Mar 9 2021, 6:17 PM

sys/i386/i386/db_trace.c
792–795	Note that this block is dropped, but can be added back if desired.

mhorne added a child revision: D29155: Introduce kdb-level watchpoint functions.Mar 9 2021, 7:12 PM

kib added inline comments.Mar 9 2021, 7:29 PM

sys/conf/files.x86
323	Does the content of the file needed for !DDB?
sys/x86/x86/dbreg.c
30	Was this code copy/pasted with trivial modifications? I really do not think that the new license is applicable to it.

mhorne added inline comments.Mar 9 2021, 7:57 PM

sys/conf/files.x86
323	No, they aren't used elsewhere yet. By the end of my patch series it can become `ddb \| gdb`.
sys/x86/x86/dbreg.c
30	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.

Harbormaster completed remote builds in B37705: Diff 85423.Mar 9 2021, 8:20 PM

kib added inline comments.Mar 10 2021, 2:14 AM

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).