kvprintf %b enhancements

[rlibby]

Summary

Make the %b formatter accept number formatting flags. It will now accept alternate form, precision, and length modifiers. It also now partially supports field width (but forces left justification).

The motivation is to make %b easier to use and behave more like %x and %o.

I was tempted to add support for right justification but as far as I could see it would have meant either more code duplication or more refactoring of kvprintf, so I decided to leave it alone for now.

Test Plan

Edited the db_show_buffer show command and checked output. This demonstrates that %#b now works.

 	db_printf("b_flags = 0x%b, b_xflags=0x%b, b_vflags=0x%b\n",
 	    (u_int)bp->b_flags, PRINT_BUF_FLAGS, (u_int)bp->b_xflags,
 	    PRINT_BUF_XFLAGS, (u_int)bp->b_vflags, PRINT_BUF_VFLAGS);
+	db_printf("b_flags = %#b, b_xflags=%#hhb, b_vflags=%#b\n",
+	    bp->b_flags, PRINT_BUF_FLAGS, bp->b_xflags, PRINT_BUF_XFLAGS,
+	    bp->b_vflags, PRINT_BUF_VFLAGS);

# dd if=/dev/zero of=/tmp/foo.dd

~KDB: enter: Break to debugger
[ thread pid 1838 tid 100113 ]
Stopped at      kdb_alt_break_internal+0x106:   movq    $0,kdb_why
db> show lockedbufs
buf at 0xfffffe00f60bd420
b_flags = 0x20000000<vmio>, b_xflags=0x2<clean>, b_vflags=0x0
b_flags = 0x20000000<vmio>, b_xflags=0x2<clean>, b_vflags=0
[...]

[dim]

I would rather dump the non-standard printf specifiers, than do actual work on them. The usage of these should be minimized, not encouraged.

Furthermore, patches to the llvm code should be submitted upstream, not in FreeBSD. We don't want to maintain changes separate from upstream anymore.

[cem]

I would rather dump the non-standard printf specifiers, than do actual work on them. The usage of these should be minimized, not encouraged.

Eh, I disagree. Especially in the kernel. %b is super useful.

[dim]

In D11284#233563, @cem wrote:

I would rather dump the non-standard printf specifiers, than do actual work on them. The usage of these should be minimized, not encouraged.

Eh, I disagree. Especially in the kernel. %b is super useful.

Well, don't expect any format checking then. External toolchains will never recognize these... :)

[rlibby]

In D11284#233558, @dim wrote:

I would rather dump the non-standard printf specifiers, than do actual work on them. The usage of these should be minimized, not encouraged.

It doesn't seem to be deprecated, a quick group shows about 170 call sites with %b, and it's specifically documented in man9.

Of course, printf(9) is not a standard C printf formatter anyway, since we don't support floats.

Furthermore, patches to the llvm code should be submitted upstream, not in FreeBSD. We don't want to maintain changes separate from upstream anymore.

Thanks, I didn't realize the printf(9) format analysis was in upstream clang. I'll drop those patches here.

[rlibby]

dim feedback: don't touch contrib

[rlibby]

Ping. I guess the question is whether the behavior change is desired, considering @dim's point.

I dropped the llvm format analysis changes, so now the patch just changes the capabilities of kvprintf %b. If this goes in then I'll try to upstream those llvm changes. I will also follow up for the corresponding gcc changes (contrib/gcc/c-format.c).

[markj]

In D11284#235335, @rlibby wrote:

Ping. I guess the question is whether the behavior change is desired, considering @dim's point.

I don't think there's much demand for a change along these lines, so the main consideration is whether you plan to make use of these extensions in future commits. Due to the lack of compiler checking, I would avoid adding %b uses outside of the existing use-cases of ddb handlers and device feature flag listing. If you're interested in using this change as part of a cleanup of some of the ddb handlers, for instance, I think it would be reasonable to commit.

I dropped the llvm format analysis changes, so now the patch just changes the capabilities of kvprintf %b. If this goes in then I'll try to upstream those llvm changes. I will also follow up for the corresponding gcc changes (contrib/gcc/c-format.c).

The in-tree gcc is quite old and isn't maintained in any real sense. It's only there for architectures that aren't supported by LLVM. I believe the eventual plan is to improve support for out-of-tree compilers to the point where the in-tree gcc can be removed, so I don't think it's worth spending any time modifying it.

[rlibby]

In D11284#235466, @markj wrote:

In D11284#235335, @rlibby wrote:

Ping. I guess the question is whether the behavior change is desired, considering @dim's point.

I don't think there's much demand for a change along these lines, so the main consideration is whether you plan to make use of these extensions in future commits. Due to the lack of compiler checking, I would avoid adding %b uses outside of the existing use-cases of ddb handlers and device feature flag listing. If you're interested in using this change as part of a cleanup of some of the ddb handlers, for instance, I think it would be reasonable to commit.

I agree with the sentiment of not wanting to spread %b beyond the debug flag printers. I was considering using %#b to replace a few hundred lines of hand-rolled ddb flag printing code (grep -R -e "comma = 1" -e "^db_print_" sys). Beyond that I don't have a grand design. I do have an idea for making it easier to write the flag strings though (something like KPRF_BFLAG(SO_ACCEPTCONN) => "\002" "SO_ACCEPTCONN").

[markj]

In D11284#235477, @rlibby wrote:

In D11284#235466, @markj wrote:

In D11284#235335, @rlibby wrote:

Ping. I guess the question is whether the behavior change is desired, considering @dim's point.

I don't think there's much demand for a change along these lines, so the main consideration is whether you plan to make use of these extensions in future commits. Due to the lack of compiler checking, I would avoid adding %b uses outside of the existing use-cases of ddb handlers and device feature flag listing. If you're interested in using this change as part of a cleanup of some of the ddb handlers, for instance, I think it would be reasonable to commit.

I agree with the sentiment of not wanting to spread %b beyond the debug flag printers. I was considering using %#b to replace a few hundred lines of hand-rolled ddb flag printing code (grep -R -e "comma = 1" -e "^db_print_" sys). Beyond that I don't have a grand design. I do have an idea for making it easier to write the flag strings though (something like KPRF_BFLAG(SO_ACCEPTCONN) => "\002" "SO_ACCEPTCONN").

Oof. I do think it would be worth simplifying that flag-printing code, so if you're planning to go ahead with that, I'll approve this change.

sys/kern/subr_prf.c

934 ↗

(On Diff #29877)

The only assignment to tmp is at the beginning of the loop - isn't this predicate always false?

[rlibby]

sys/kern/subr_prf.c

934 ↗

(On Diff #29877)

tmp isn't modified, but retval is. tmp here is the old count, and retval is the current count. It's a little obscure because retval is modified by the PCHAR macro.

[markj]

Looks ok to me, and approved.

sys/kern/subr_prf.c

934 ↗

(On Diff #29877)

Ah, got it.