Page MenuHomeFreeBSD

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

Authored by kp on Oct 2 2024, 9:51 AM.
Tags
None
Referenced Files
F104272696: D46871.diff
Thu, Dec 5, 4:39 PM
Unknown Object (File)
Sun, Dec 1, 12:20 PM
Unknown Object (File)
Sat, Nov 30, 1:09 AM
Unknown Object (File)
Tue, Nov 26, 7:29 AM
Unknown Object (File)
Fri, Nov 15, 2:07 PM
Unknown Object (File)
Thu, Nov 14, 7:15 AM
Unknown Object (File)
Tue, Nov 12, 4:37 AM
Unknown Object (File)
Oct 15 2024, 4:13 AM
Tokens
"Yellow Medal" token, awarded by mhorne.

Details

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

Event Timeline

kp requested review of this revision.Oct 2 2024, 9:51 AM
kp created this revision.
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

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 always have a GENERIC-DEBUG for stable branches, the content is similar. +1 for this.

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

More comment fixes & remove unwanted changes in i386/MINIMAL

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 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?

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.

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

sys/conf/std.debug
21

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.

32

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

sys/conf/std.debug
32

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

kp marked 3 inline comments as done.

Remove KCOV & CAM_DEBUG_FLAGS

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
9

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

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

This revision is now accepted and ready to land.Oct 8 2024, 11:33 PM