Page MenuHomeFreeBSD
Differential D46871

sys: add conf/std.debug, generic debugging options
ClosedPublic
Actions

Authored by kp on Oct 2 2024, 9:51 AM.
Tags
None
Referenced Files
F106165432: D46871.id144297.diff
Thu, Dec 26, 11:27 AM
F106164190: D46871.id144247.diff
Thu, Dec 26, 10:59 AM
Unknown Object (File)
Mon, Dec 23, 7:57 PM
Unknown Object (File)
Sat, Dec 14, 11:55 PM
Unknown Object (File)
Fri, Dec 13, 8:40 PM
Unknown Object (File)
Mon, Dec 9, 3:36 AM
Unknown Object (File)
Thu, Dec 5, 4:39 PM
Unknown Object (File)
Sun, Dec 1, 12:20 PM
View All 22 Files
Subscribers
jhibbits
olce
zlei
riscv
Tokens
"Yellow Medal" token, awarded by mhorne.

Details

Reviewers
thj
emaste
imp
markj
jhb
olce
andrew
manu
Commits
rG0d4081ed6914: sys: add conf/std.debug, generic debugging options
Summary

The new sys/conf/std.debug contains the list of debugging options
enabled by default in -CURRENT, so they don't need to be listed
individually in every kernel config.

Introduce *-DEBUG variants of the major kernel configs.

(cherry picked and modified from commit 4f8f9d708e6a4143f3b178bfab10d0a9b75ba2fe)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59800
Build 56685: arc lint + arc unit

Event Timeline

kp requested review of this revision.Oct 2 2024, 9:51 AM
kp created this revision.
olce accepted this revision.Oct 2 2024, 9:57 AM
olce added a subscriber: olce.

Thanks!

sys/conf/std.debug
3
This revision is now accepted and ready to land.Oct 2 2024, 9:57 AM
zlei added a subscriber: zlei.Oct 3 2024, 3:16 PM
markj added a comment.Oct 3 2024, 11:13 PM

This patch is against stable/14, not main

I very much want this (and for release media to start including a kernel.debug by default when possible, cc releng), but sys/conf/std.debug already exists on main. In particular, you've added a few extra _DEBUG options here which aren't in GENERIC. I think that's probably fine, but ideally we'd start by MFCing the commit which added std.debug.

zlei added a comment.Oct 4 2024, 10:00 AM

I always have a GENERIC-DEBUG for stable branches, the content is similar. +1 for this.

kp added a comment.Oct 4 2024, 1:28 PM
In D46871#1069247, @markj wrote:

This patch is against stable/14, not main

I very much want this (and for release media to start including a kernel.debug by default when possible, cc releng), but sys/conf/std.debug already exists on main. In particular, you've added a few extra _DEBUG options here which aren't in GENERIC. I think that's probably fine, but ideally we'd start by MFCing the commit which added std.debug.

I can (and should) start from that commit, but it changes the GENERIC configs, which isn't quite what we want on stable/14,stable/13.
I've done a cherry-pick along with major modifications and will update this review with that in a few minutes.

kp updated this revision to Diff 144245.Oct 4 2024, 1:30 PM
kp retitled this revision from amd64: add GENERIC-DEBUG kernel config to sys: add conf/std.debug, generic debugging options.
kp edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Oct 4 2024, 1:30 PM
Herald added a reviewer: andrew. · View Herald TranscriptOct 4 2024, 1:30 PM
Herald added a reviewer: andrew. · View Herald Transcript
Herald added a reviewer: manu. · View Herald Transcript
Herald added subscribers: riscv, jhibbits. · View Herald Transcript
kp updated this revision to Diff 144246.Oct 4 2024, 1:31 PM

Fix RISC-V comment.

kp updated this revision to Diff 144247.Oct 4 2024, 1:33 PM

More comment fixes & remove unwanted changes in i386/MINIMAL

markj added a comment.Oct 4 2024, 2:01 PM

Do these configs automatically get built with tinderbox and universe make targets? If so, this looks good to me aside from some nits I commented on inline.

sys/amd64/conf/GENERIC-DEBUG
4

The indentation here looks off.

11

Extra newline, here and in some of the other configs below.

sys/amd64/conf/NOTES
164

KCSAN appears twice.

sys/conf/std.debug
18

You might add commented out lines referencing the other subsystem debug options you had originally.

bz added a subscriber: bz.Oct 4 2024, 8:58 PM
bz added inline comments.
sys/conf/std.debug
18

std.nodebug also has:

# Net80211 debugging
nooptions       IEEE80211_DEBUG

# USB debugging
nooptions       USB_DEBUG
nooptions       HID_DEBUG

# CAM debugging
nooptions       CAMDEBUG
nooptions       CAM_DEBUG_FLAGS

is there a reason to not add these as well (apart from CAM_DEBUG_FLAGS)? That said, I am not sure why this file shows up as new here?

kp added inline comments.Oct 4 2024, 9:05 PM
sys/conf/std.debug
18

That said, I am not sure why this file shows up as new here?

This is for stable/14 (and later I'll look at a variant for stable/13 too). std.debug already exists in main, but not on the stable branches.

bz removed a subscriber: bz.Oct 4 2024, 9:39 PM
kp updated this revision to Diff 144297.Oct 5 2024, 9:20 AM
  • fix whitespace
  • add a few more (some commented out) debug options

I've also done a make tinderbox and confirmed that builds these new kernel configs.

markj added inline comments.Oct 5 2024, 3:19 PM
sys/conf/std.debug
22

KCOV and COVERAGE aren't really debug options, they're used for code coverage instrumentation. COVERAGE is the code coverage sanitizer, KCOV is a small device interface used by syzkaller to enable coverage collection from userspace.

I would just remove them from here.

33

It's not clear to me that we want these enabled by default. They aren't enabled in GENERIC on main.

imp added inline comments.Oct 5 2024, 4:26 PM
sys/conf/std.debug
33

Keep CAMDEBUG. It's fairly low overhead. Lose the FLAGS. It sets the default only and needs a value.

kp updated this revision to Diff 144360.Oct 7 2024, 8:19 AM
kp marked 3 inline comments as done.

Remove KCOV & CAM_DEBUG_FLAGS

Harbormaster completed remote builds in B59800: Diff 144360.Oct 7 2024, 8:19 AM
markj accepted this revision.Oct 7 2024, 11:49 AM
This revision is now accepted and ready to land.Oct 7 2024, 11:49 AM
imp requested changes to this revision.Oct 8 2024, 4:24 PM

I'm curious why BUF_TRACKING is on this list... It changes the KBI so would create a bigger problem with not being able to load .ko's than it would solve. It's rarely useful to tracking down issues and introduces a decent amount of overhead to boot. So on the whole, I think we'd be better off without it, but maybe there's some good reason to include it.

sys/amd64/conf/NOTES
166

this is only tangentially related. It would be better as a separate commit imho.

sys/conf/std.debug
8

I'm not sure this is a good idea. I think it would slow things down too much. We don't normally enable it, and it's quite intrusive.
I'm not even sure BUF_TRACKING is a good idea. It changes the ABI in weird ways since it alters the layout of struct buf making it impossible to use packages that have .ko's that deal with buf.

This revision now requires changes to proceed.Oct 8 2024, 4:24 PM
kp added a comment.Oct 8 2024, 8:51 PM
In D46871#1071719, @imp wrote:

I'm curious why BUF_TRACKING is on this list...

It's also in sys/conf/std.debug on main.

It changes the KBI so would create a bigger problem with not being able to load .ko's than it would solve.

We're going to have that problem anyway, between DEBUG and NODEBUG builds. I'm not sure why that's a concern.

It's rarely useful to tracking down issues and introduces a decent amount of overhead to boot. So on the w>hole, I think we'd be better off without it, but maybe there's some good reason to include it.

"It's enabled on main" is the extent of my reasoning. I don't particularly care about individual debug features, to be honest. I mostly only ever care about INVARIANTS and WITNESS, because those two are 95% of the value of a DEBUG build.
So I don't have particularly strong views here. I'm primarily interested in making it easier to build debug versions on stable branches, because it'll be more useful to build and test those than release versions.

markj added a comment.Oct 8 2024, 11:01 PM
In D46871#1071775, @kp wrote:
In D46871#1071719, @imp wrote:

I'm curious why BUF_TRACKING is on this list...

It's also in sys/conf/std.debug on main.

It changes the KBI so would create a bigger problem with not being able to load .ko's than it would solve.

We're going to have that problem anyway, between DEBUG and NODEBUG builds. I'm not sure why that's a concern.

It's rarely useful to tracking down issues and introduces a decent amount of overhead to boot. So on the w>hole, I think we'd be better off without it, but maybe there's some good reason to include it.

"It's enabled on main" is the extent of my reasoning. I don't particularly care about individual debug features, to be honest. I mostly only ever care about INVARIANTS and WITNESS, because those two are 95% of the value of a DEBUG build.
So I don't have particularly strong views here. I'm primarily interested in making it easier to build debug versions on stable branches, because it'll be more useful to build and test those than release versions.

The KBI concerns aren't very relevant in practice:

  • most fields in struct buf are not affected, except the page array, and most consumers have no business poking around there,
  • the size of struct buf is already dynamic, so consumers can't deal with arrays of them,
  • buffer cache consumers never allocate bufs, that's handled by vfs_bio.c,
  • there aren't many (any?) 3rd party modules that people care about that manipulate bufs.
imp added a comment.Oct 8 2024, 11:33 PM
In D46871#1071808, @markj wrote:
In D46871#1071775, @kp wrote:
In D46871#1071719, @imp wrote:

I'm curious why BUF_TRACKING is on this list...

It's also in sys/conf/std.debug on main.

It changes the KBI so would create a bigger problem with not being able to load .ko's than it would solve.

We're going to have that problem anyway, between DEBUG and NODEBUG builds. I'm not sure why that's a concern.

It's rarely useful to tracking down issues and introduces a decent amount of overhead to boot. So on the w>hole, I think we'd be better off without it, but maybe there's some good reason to include it.

"It's enabled on main" is the extent of my reasoning. I don't particularly care about individual debug features, to be honest. I mostly only ever care about INVARIANTS and WITNESS, because those two are 95% of the value of a DEBUG build.
So I don't have particularly strong views here. I'm primarily interested in making it easier to build debug versions on stable branches, because it'll be more useful to build and test those than release versions.

The KBI concerns aren't very relevant in practice:

  • most fields in struct buf are not affected, except the page array, and most consumers have no business poking around there,
  • the size of struct buf is already dynamic, so consumers can't deal with arrays of them,
  • buffer cache consumers never allocate bufs, that's handled by vfs_bio.c,
  • there aren't many (any?) 3rd party modules that people care about that manipulate bufs.

Hmmm... There are several fs modules that we care about. However, I don't think any of them manipulate b_pages, which is the only visible thing normally... and I don't think that there's any that hook into that #ifdef. So I'll withdraw my objection. I do think that it is a bit too high overhead, but I'll take a wait-and-see approach then.

imp accepted this revision.Oct 8 2024, 11:33 PM
This revision is now accepted and ready to land.Oct 8 2024, 11:33 PM
Closed by commit rG0d4081ed6914: sys: add conf/std.debug, generic debugging options (authored by kp). · Explain WhyOct 9 2024, 9:17 AM
This revision was automatically updated to reflect the committed changes.
kp added a commit: rG0d4081ed6914: sys: add conf/std.debug, generic debugging options.
mhorne awarded a token.Oct 9 2024, 1:36 PM

Revision Contents
Changeset List

PathSize
sys/
amd64/
conf/
9 lines
9 lines
11 lines
arm/
conf/
9 lines
arm64/
conf/
9 lines
conf/
29 lines
i386/
conf/
9 lines
powerpc/
conf/
9 lines
9 lines
9 lines
riscv/
conf/
9 lines

Diff 144360

View Options

sys/amd64/conf/GENERIC-DEBUG

Loading...
View Options

sys/amd64/conf/MINIMAL-DEBUG

Loading...
View Options

sys/amd64/conf/NOTES

Loading...
View Options

sys/arm/conf/GENERIC-DEBUG

Loading...
View Options

sys/arm64/conf/GENERIC-DEBUG

Loading...
View Options

sys/conf/std.debug

Loading...
View Options

sys/i386/conf/GENERIC-DEBUG

Loading...
View Options

sys/powerpc/conf/GENERIC-DEBUG

Loading...
View Options

sys/powerpc/conf/GENERIC64-DEBUG

Loading...
View Options

sys/powerpc/conf/GENERIC64LE-DEBUG

Loading...
View Options

sys/riscv/conf/GENERIC-DEBUG

Loading...