Add custom debug printing for your asserts

[rincebrain]

Motivation and Context

(Recreated because it wouldn't let me reopen #14466.)

I kept rewriting assertions into if(assert condition) { print something; trip assertion; } and went "this probably works".

So now I want to see if the CI explodes on Debian 8 or something.

(Before merge I'd probably refactor the non-f versions into being in terms of the f versions, but this way I can test a much smaller blast radius at a time...also suggestions for better naming welcome.)

(...I also flattened freebsd's debug.h and linux's after noticing they were more or less the same file, but that's mostly orthogonal and just in here because I also want to make sure that compiles everywhere.)

Description

Gives all the ASSERT/VERIFY macros an "f" suffix variant that adds "STR, ..." arguments such that you could do, say ASSERT3Pf(hdr->b_l1hdr.b_pabd, !=, NULL, "hdr %px buf %px", hdr, buf); and get an error trip like VERIFY3(hdr->b_l1hdr.b_pabd != NULL) failed (0000000000000000 != 0000000000000000) hdr 0000000000000000 buf 0000000000000000

The only big caveat is I don't think you can do something like ((void) __VA_ARGS__) so the print varargs won't always get executed (because ASSERT normally grounds out in ((void) x) on non-DEBUG builds), so don't put side effect things in there you need to always run...but that's not a reason not to have it, just a warning.

How Has This Been Tested?

I mean, I compiled it once, what could go wrong, right?

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Performance enhancement (non-breaking change which improves efficiency)
• [ ] Code cleanup (non-breaking change which makes code smaller or more readable)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• [ ] Documentation (a change to man pages or other documentation)

Checklist:

• [ ] My code follows the OpenZFS code style requirements.
• [ ] I have updated the documentation accordingly.
• [x] I have read the contributing document.
• [ ] I have added tests to cover my changes.
• [ ] I have run the ZFS Test Suite with this change applied.
• [ ] All commit messages are properly formatted and contain Signed-off-by.

[robn]

Oh, I love this. I write a jank variant of this nearly every day. Nice work!

If sys/debug.h is going to be "the same" on both platforms, should it just be moved to include/sys?

I'll also say that the lowercase f makes me twitch, both mixed caps and macros that aren't caps. I probably would have written VERIFYF etc. But I don't know how strongly I feel about that, and I'm not really gonna argue I think.

[rincebrain]

Oh, I love this. I write a jank variant of this nearly every day. Nice work!

If sys/debug.h is going to be "the same" on both platforms, should it just be moved to include/sys?

I was a little worried it might get pulled in by libspl if I do that, and I could coalesce both of those too, but that was why I didn't, was that I mostly wanted to get feedback on the design, and figured that could be implemented later if it worked and people liked the idea.

I'll also say that the lowercase f makes me twitch, both mixed caps and macros that aren't caps. I probably would have written VERIFYF etc. But I don't know how strongly I feel about that, and if I'm not really gonna argue I think.

I don't really like it either, if I'm honest, but since all the uppercase was overloaded with pointer/unsigned/etc, lowercase seemed like the most expressive way to make it clear this wasn't just a different element type. Not that we use floating point around much, but that was why.

Suggestions on alternatives welcome.

[behlendorf]

I'm all for this. Thanks for reviving this work.

For what it's worth, I'd lean slightly towards ASSERTF/VERIFYF instead of ASSERTf/VERIFYf. Lower case letters in macros and mixed case make me a bit queasy, but I could learn to live with it.

I was a little worried it might get pulled in by libspl.

It would be nice if these shared a common file, but it looks to me like that would currently be a problem. We've bumped in to this in a few other places so a generic way to handle it would be good. Maybe a new include/os/generic/spl/ directory? In the short term we could use a symlink, but that's icky too. Either way, that's the kind of thing we could leave to follow up PR.

[behlendorf]

@rincebrain if you can rebase this and sort out the style warnings I'll go ahead and merge it. This is such a nice quality of life improvement I'd hate for it to get hung up on any ASSERTF/VERIFYF vs ASSERTf/VERIFYf minutia. Let's merge it, live with it for a bit, and tweak it if needed.

[rincebrain]

Honestly, it wasn't a disagreement about the f versus F for me, I just forgot I never replied or updated this.

I'll go rebase and push.

[amotin]

I'd hate for it to get hung up on any ASSERTF/VERIFYF vs ASSERTf/VERIFYf minutia

I think it should be much easier to fix the capitalization now than remember about this case oddity forever after.

[rincebrain]

Looking at it quickly, I'm debating what to do about the %p/%px usage, since the x print is kind of ugly on non-Linux platforms, but I don't really want to go around plumbing in a "print less gross" PTR_PRINTF or something for this. Maybe a followup.

[behlendorf]

It seems to me that %px is appropriate here, even if it might be ugly on non-Linux platforms. A followup PR is always an option if it becomes a real annoyance.

[behlendorf]

@rincebrain if you can sort out the remaining build errors I think we'd all love to get a version of this integrated.

[rincebrain]

...fascinatingly, my branch locally has no pending changes and even after a make distclean and configure, builds. I wonder what's going on.

[rincebrain]

Not really sure what happened, but a fresh clone from the repo into a new dir and the same configure+make breaks as in the repo, now, so that was simple to debug.

[behlendorf]

Strange, but this looks good to me as long as the CI is happy this time around.

[rincebrain]

I think this is good to go and the centos 7 failure is unrelated.

I just squashed the two fix commits and pushed to see what happens.

[rincebrain]

The FBSD tests seem to be the same failure described in #15934, the CentOS 9 tests seem to be bclone broken for every PR atm, and the others seem to be flaky tests, I think?

Someone check me, by all means, but they all seem to be unrelated to this change.

[behlendorf]

@rincebrain can I talk you one to one last rebase, then we can get this merged.