kernel macro cleanliness, first pass

[rlibby]

Summary

This is a set of related non-functional patches to clean up some macro
issues which can potentially lead to misleading expansions. This patch
is (unfortunately) pretty boring, and as far as I know none of these
caused an actual issue. I searched especially for bogus trailing
semicolons and missing do/while loops. I also enclosed some macro
parameters in parentheses in a few places where I was editing the macro
anyway or something nearby.

These patches are all in sys and outside sys/dev. I also avoided arch
code in this set of patches to facilitate testing (see below).

I would intend to commit these essentially file-by-file, but
consolidating to one review to save people's time. I can split out or
drop any of these as appropriate.

---

Individual logs:

14afaa180dbb audit_bsm_bd.c: macro cleanliness (terminating semicolon)
10f6a844ad04 netgraph/ng_tcpmss.c: macro cleanliness
c6a403aa6494 net/radix.h: macro cleanliness
952941ba4a98 vnet: macro cleanliness (terminating semicolons)
c626be52827f netinet/in_var.h: macro cleanliness
6d996cc43881 netgraph/ng_pipe.c: FIFO_VTIME_SORT: macro cleanliness
52ade0fb9143 zlib.c: macro cleanliness
2afb7c1e9bd0 scsi_low.h: macro cleanliness
2697da421ca1 intel_dmar.h: macro cleanliness
4ff39aaa1f47 sys/kern: macro cleanliness (terminating semicolons)
cf7f9938d358 kern_clocksource.c: macro cleanliness
993a62feac87 vm_map.c: macro cleanliness
b370e18bceab sys/sys/ata.h: macro cleanliness (terminating semicolons)
4efa00de7b06 sys/sys/sched.h: macro cleanliness (terminating semicolon)
aaa0652763f4 sys/sys/time.h: FREQ2BT macro cleanliness

Test Plan

buildkernel

buildkernel before and after, and then diffing the resulting objects.
This isn't a literal diff -rq since line numbers changed etc. Instead
it was

find clang-1 -type f | xargs -I%% sh -c '~/objdiff.sh %% $(echo %% | sed s/clang-1/clang-2/)'

Which is, call objdiff.sh to compare each file in the before and after
object trees. objdiff.sh diffs objects after --strip-debug'ing them:

#!/bin/sh

# Diff elf objects without debug info.

set -e

# Skip the rest if a regular diff shows no diff.
if diff -q "$1" "$2" > /dev/null; then
	exit 0;
fi

# Copy the paths of the compared files so that they have informative names.
DIRNAME1=$(dirname "$1")
DIRNAME2=$(dirname "$2")
TMPDIR=$(mktemp -d)
mkdir -p "$TMPDIR/$DIRNAME1"
mkdir -p "$TMPDIR/$DIRNAME2"
objcopy --strip-debug "$1" "$TMPDIR/$1"
objcopy --strip-debug "$2" "$TMPDIR/$2"

# Temporarily disable set -e, we want to record the return value.
set +e
diff -q "$TMPDIR/$1" "$TMPDIR/$2"
RV=$?
set -e

if [ $RV -eq 0 ]; then
	rm -rf "$TMPDIR"
fi
exit $RV

And the result is just a difference in vers.o:

$ find clang-1 -type f | xargs -I%% sh -c '~/objdiff.sh %% $(echo %% | sed s/clang-1/clang-2/)'
objcopy:clang-1/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/vers.c: File format not recognized
Files /tmp/tmp.7bc8yEVG3F/clang-1/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/vers.o and /tmp/tmp.7bc8yEVG3F/clang-2/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/vers.o differ
Files /tmp/tmp.1aHMMasflr/clang-1/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel.full and /tmp/tmp.1aHMMasflr/clang-2/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel.full differ
Files /tmp/tmp.7Rsf9mXNhr/clang-1/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel and /tmp/tmp.7Rsf9mXNhr/clang-2/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel differ
$ objdump -s /tmp/tmp.1aHMMasflr/clang-2/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel.full > /tmp/kernel.full.2.txt
$ diff /tmp/kernel.full.1.txt /tmp/kernel.full.2.txt
2c2
< /tmp/tmp.1aHMMasflr/clang-1/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel.full:     file format elf64-x86-64
---
> /tmp/tmp.1aHMMasflr/clang-2/usr/src/freebsd-branches/macro-cleanup/sys/GENERIC/kernel.full:     file format elf64-x86-64
1463498,1463499c1463498,1463499
<  ffffffff81a55450 74204f63 74203231 2031353a 30323a32  t Oct 21 15:02:2
<  ffffffff81a55460 31205044 54203230 31370000 00000000  1 PDT 2017......
---
>  ffffffff81a55450 74204f63 74203231 2031353a 31373a32  t Oct 21 15:17:2
>  ffffffff81a55460 33205044 54203230 31370000 00000000  3 PDT 2017......
1463506c1463506
<  ffffffff81a554d0 74203231 2031353a 30323a32 31205044  t 21 15:02:21 PD
---
>  ffffffff81a554d0 74203231 2031353a 31373a32 33205044  t 21 15:17:23 PD

[mjoras]

The superfluous parentheses stuff is sort of nitpicky, but I do feel that at least in the case where the expansion is surrounded by commas then it should be dropped since comma operators / separators have the lowest possible precedence.

I noticed other macro style problems in the files you touched, but we don't want to go too crazy I suppose :).

sys/kern/sysv_sem.c

129

Superfluous parentheses around how (which you didn't add).

sys/libkern/zlib.c

2198–2199

Parenthesize top and s.

sys/netgraph/ng_tcpmss.c

144–147

I'm not crazy about this being on one line in the first place, so maybe split it?

sys/netinet/in_var.h

143

This parenthetical is superfluous due to the comma.

158

Same nit re: comma.

sys/vm/vm_map.c

1676

Parenthesize startaddr and entry.

1762

entry->end is parenthesized incorrectly.

1763

Superfluous parentheses. You didn't add them but it is a changed line.

[rlibby]

In D12753#264690, @mjoras wrote:

The superfluous parentheses stuff is sort of nitpicky, but I do feel that at least in the case where the expansion is surrounded by commas then it should be dropped since comma operators / separators have the lowest possible precedence.

No problem. Made the requested changes, and a few more along the same lines.

I noticed other macro style problems in the files you touched, but we don't want to go too crazy I suppose :).

Yes ... I know.

I was trying to narrow down the macros which would trip a couple simple searches I have ( https://people.freebsd.org/~rlibby/macro-cleanup/ ) since those at least are readily measurable. It is hard to search for proper parenthesization. Do let me know if you have any ideas for more searches.

Updated patch coming, just running through the object diffing again.

sys/libkern/zlib.c

2198–2199

I tacked on the below smaller macro too...

[rlibby]

mjoras feedback: parenthesization fixes

Also, add on sys/sys/buf.h: clrbuf

[kib]

sys/kern/kern_clocksource.c

84

Looks ok.

sys/kern/subr_kobj.c

65

Looks ok.

sys/kern/subr_vmem.c

199

Looks ok.

sys/kern/sysv_sem.c

129

Why superfluous ?

sys/security/audit/audit_bsm_db.c

103

Looks fine.

sys/sys/ata.h

577

Looks fine.

sys/sys/buf.h

464 ↗

(On Diff #34225)

It seems that there are just two uses of this macro. Perhaps inline it and remove the definition, the macro is useless.

sys/sys/sched.h

194

Looks good.

sys/sys/time.h

511

Looks good.

sys/vm/vm_map.c

174

Looks good.

1677

I think that you should add () around all args.

1763

I think it is exactly in reverse, the () are needed there. What if an expression like a=2,b=3 passed as an argument ?

sys/x86/iommu/intel_dmar.h

539

Looks fine.

[mjoras]

sys/kern/sysv_sem.c

129

Is it possible, without the parentheses, for this to change meaning relative to a function call? If not then it is technically superfluous.

sys/vm/vm_map.c

1677

Again, it's not possible for it to change meaning when the args are used separated by commas.

1763

I don't follow. They would have to be passed with parens added by the caller vm_map_clip_end(map, (a=2,b=3), endaddr), otherwise the preproccessor will interpret them as macro arguments. Adding more parentheses in the definition of the macro does not help.

[mjoras]

I'd rather not turn my (perhaps radical) parentheses opinions into a bikeshed. At the least I request not adding new ones that aren't needed, but if kib or someone else prefers they stay I won't get in the way.

[mjoras]

sys/vm/vm_map.c

1763

Unless you meant something where you #define argx a=2,b=2 ... vm_map_clip_end(map, argx, endaddr);, but that seems a rather contrived situation.

[rlibby]

sys/sys/buf.h

464 ↗

(On Diff #34225)

Hmm, and the use in udf_strategy looks questionable to me. Shouldn't the error cases be setting b_error / BIO_ERROR instead? But maybe there's more to it than it looks like.

sys/vm/vm_map.c

1763

It seems like a bad idea but we do have some examples of macros defined as multiple arguments, e.g.:

netgraph.h: _NN_
contrib/octeon-sdk/cvmx-fau.h: CVMX_FAU_BITS_*

However the commas in these macros are intended to be argument separators, not comma operators, so they are not breaking the meaning in that sense.

I am ambivalent about the parens between commas question. But I think whichever way, it should be a simple and clear recommendation we could add to style(9), "parenthesize all instances of macro parameter names" and possibly "except when the instance is surrounded by parentheses or commas, or is the variable argument of a variadic macro."

Wrt this patch, if the question is not settled, I'd rather maintain the status quo for now (including the superfluous parens).

Thoughts?

[kib]

sys/vm/vm_map.c

1763

I agree that keeping it as is for now is the best route.

[mjoras]

sys/vm/vm_map.c

1763

Sure, let's keep it as it is now and just not add any additional superfluous parentheses. I agree that there should be some quorum about the issue so that it can be added as a recommendation to style(9).

[rlibby]

sys/sys/buf.h

464 ↗

(On Diff #34225)

I'm just going to drop the change to clrbuf for now and follow up on it. I want to understand if what's going on in udf_strategy is in fact bogus anyway, and whether vfs_bio_clrbuf may want to KASSERT(buf_mapped()) and/or whether that is already implied.

sys/vm/vm_map.c

1763

Okay, I am reverting the questionable ones to the status quo... except that I am now making vm_map_clip_start / vm_map_clip_end consistent with each other (by adding "superfluous" parens to vm_map_clip_start for now--sorry @mjoras :).

[rlibby]

Review feedback: avoid touching "superfluous" macro parens for now

Also leave clrbuf out for follow up.