build: Reduce the cost of supporting NO<INET|INET6|IP>* variants of the kernel.

[melifaro]

Summary

Currently these kernels share the same strict set of the warnings treated as errors.
This approach comes with a cost: there are more than 100 places in the kernel where variable declarations happen under one of the INET/INET6 defines (or both). Similarly, same happens with IP/IPv6-only functions - they have to be put under the define.
These defines do not improve the code size, as the compilers optimise out unused variables an local functions anyway.
These defines makes the code worse - by making it less readable.
These defines makes the code harder to write, as the developer have to check that all INET* permutation builds.

Address it by removing error-yelling for unused functions and variables if either of INET or INET6 options is not set.
Remove some defines from the networking code that are not required anymore.

[imp]

I'm not sure I like this... I'm not a fan of disabling warnings since it could hide other issues that creep in...

[emaste]

sys/conf/kern.pre.mk

159–162

unexpected change?

[kp]

In D40183#914803, @imp wrote:

I'm not sure I like this... I'm not a fan of disabling warnings since it could hide other issues that creep in...

It seems reasonable in this case, because it only applies if INET or INET6 are not set. So we'll still get warned about unused variables in the much more common case of kernels that do include INET and INET6.

I'd certainly agree with your objection if the proposal were to remove the unused function/variable warnings entirely, but I don't think they add much for NOINET/NOINET6 kernels.

[melifaro]

Cleanup.

[melifaro]

In D40183#914803, @imp wrote:

I'm not sure I like this... I'm not a fan of disabling warnings since it could hide other issues that creep in...

I don't like disabling warning either - they help maintain the code at better shape. Here we don't disable warnings for GENERIC (and any other config which has INET and INET6 compiled) so we should get all benefits provided by the current set of warnings.
Not failing on unused-variable, unused-but-set-variable and unused-function happens only for the NOINET[6]/NOIP code (otherwise it'll be caught in GENERIC). I don't see any issues that could come from having these 3 flags enabled. Maybe I'm missing something - so happy to discuss the concerning examples & address them.

[melifaro]

Good example happening right now - ac6dd012590e01f6a5fef490d52ffb4a6ca97798 and c98146ae229cc60834bfbecc229f2f7725e458d0 wouldn't be required with this change in place.

[jhb]

I feel like the worst offenders are really when you have to #ifdef for both for LINT-NOIP. Another tact we might consider is just axeing LINT-NOIP. I can see some value in LINT-NOINET so an INET6-only kernel remains clean, but I see less value in LINT-NOIP and I feel like more of the cost is for that config.

I think rather than using evil greps in the build glue, I'd rather just add explicit lines to LINT-NO* that adjust the warnings FWIW if we want to consider doing this.

[melifaro]

In D40183#915476, @jhb wrote:

I feel like the worst offenders are really when you have to #ifdef for both for LINT-NOIP. Another tact we might consider is just axeing LINT-NOIP. I can see some value in LINT-NOINET so an INET6-only kernel remains clean, but I see less value in LINT-NOIP and I feel like more of the cost is for that config.

Typically, yes, it's the worst. However, unused functions also play a notable role. Re LINT-NOIP - I ran into some folks doing builds for embedded systems & complaining when it's broken.

I think rather than using evil greps in the build glue, I'd rather just add explicit lines to LINT-NO* that adjust the warnings FWIW if we want to consider doing this.

Thank you for the feedback!
There are tons of these greps in the kern.pre.mk so I assumed that's known acceptable evil :-) I don't have any opinion about the implementation.
I started with the explicit lines in LINT*, actually, but ran into a problem with extending CONF_CFLAGS. I'll retry tomorrow and publish the updated diff if I get it working.

[melifaro]

In D40183#915542, @melifaro wrote:

In D40183#915476, @jhb wrote:

I feel like the worst offenders are really when you have to #ifdef for both for LINT-NOIP. Another tact we might consider is just axeing LINT-NOIP. I can see some value in LINT-NOINET so an INET6-only kernel remains clean, but I see less value in LINT-NOIP and I feel like more of the cost is for that config.

Typically, yes, it's the worst. However, unused functions also play a notable role. Re LINT-NOIP - I ran into some folks doing builds for embedded systems & complaining when it's broken.

I think rather than using evil greps in the build glue, I'd rather just add explicit lines to LINT-NO* that adjust the warnings FWIW if we want to consider doing this.

Thank you for the feedback!
There are tons of these greps in the kern.pre.mk so I assumed that's known acceptable evil :-) I don't have any opinion about the implementation.
I started with the explicit lines in LINT*, actually, but ran into a problem with extending CONF_CFLAGS. I'll retry tomorrow and publish the updated diff if I get it working.

The problem with the explicit lines in LINT that I see is that the actual folks who use noinet(6) combinations don’t use LINT configs. So, by doing changes on the LINT level we’ll remove useless reporting on the ci side. The actual users will still have to change their configs..
Thoughts?

[melifaro]

I would like to try to have another iteration of the discussion :-)

@imp, @jhb: you expressed concerns about the implementation. Do you think that _conceptually_ reducing warnings in the derived (e.g. GENERIC -INET or -INET6 or both) kernels have the value w.r.t developer-effort?
If there is such benefit, do you see any potential implementation changes that would allow this diff to move forward?

Thank you.