clang vs gcc warning flags

Wed Jan 6 21:55:53 UTC 2021

On Wed, Jan 6, 2021 at 1:43 PM Warner Losh <imp at bsdimp.com> wrote:
>
>
>
> On Wed, Jan 6, 2021, 2:34 PM Ryan Libby <rlibby at freebsd.org> wrote:
>>
>> On Wed, Jan 6, 2021 at 12:38 PM Warner Losh <imp at bsdimp.com> wrote:
>> >
>> >
>> >
>> > On Wed, Jan 6, 2021 at 12:53 PM Ryan Libby <rlibby at freebsd.org> wrote:
>> >>
>> >> One of the more annoying things about keeping the gcc build going is the
>> >> set of warnings that gcc acts on but clang only recognizes for
>> >> compatibility.  As a common example, -Wredundant-decls has no effect
>> >> in clang, but will break the gcc build.  There are a couple dozen such
>> >> flags [1][2], and a few of them are in our default set of warnings, such
>> >> as in sys/conf/kern.mk, these ones in CWARNFLAGS:
>> >>
>> >>  - Wredundant-decls
>> >>  - Wnested-externs
>> >>  - Wmissing-include-dirs
>> >>
>> >> additionally some warnings are explicitly disabled for clang, but not
>> >> for gcc in CWARNEXTRA:
>> >>
>> >>  - Wempty-body
>> >>  - Wunused-function
>> >>
>> >> Similarly, in share/mk/bsd.sys.mk:
>> >>
>> >>  - Winline (although, Wno-error'd)
>> >>  - Wnested-externs
>> >>  - Wredundant-decls
>> >>  - Wold-style-definition
>> >>
>> >> So I suggest we harmonize these somewhat.
>> >>
>> >>  - Wnested-externs I just do not understand.  We have specified this
>> >>    warning flag for some 25 years but to me it seems completely without
>> >>    value.  I suggest we just delete it.
>> >
>> >
>> >
>> > It's to prevent declaring functions inside of functions. why is that bad? It only has scope of the function, and we've decided that's not something we want in the tree... I'd say that the fact we don't have any in the tree shows that it's useful at its intended purpose.
>> >
>> > What code is causing issues here?
>> >
>>
>> Well, but do we really care specifically about declaring extern
>> functions (or variables) inside of functions?  Is that worse than the
>> usual fix, moving them just outside the function to file scope?
>
>
> Yes. We have in the past. It's something we've judged to be wrong. Functions should be declared in headers, lest they be declared differently in different places. That is the harm this guards against. That is far from benign.
>
>> Regarding _defining_ functions inside of functions, I agree we don't
>> want that.  But -Wnested-externs doesn't warn about that; gcc produces
>> no warning for this with -Wnested-externs:
>>
>> void f(void) { void g(void){}; g(); }
>>
>> (Gcc produces a warning for that with -pedantic, which we don't use; and
>> clang produces an error even with no warning flags.)
>>
>> How I've actually seen "nested externs" used is usually to make
>> variables visible without including headers, for one reason or another.
>
>
> Generally that's really poor design.
>
>> Here are a couple examples:
>> https://cgit.freebsd.org/src/commit/?id=d576ccdf01a4c6f8f02e8ed7e72290c729d68de6
>> https://cgit.freebsd.org/src/tree/sys/contrib/openzfs/module/zfs/arc.c#n4721
>>
>> As far as I understand, this actually does limit the scope of the
>> visibility of the extern declaration.  I am not an expert in the
>> standard, I could be mistaken.
>
>
> The problem comes when the signatures of these functions change....
>

Okay, I take your points and I will leave -Wnested-externs alone.  I
guess as a final point, I would just suggest that non-nested externs at
file scope in .c files seems to be just as harmful as nested externs.
However, neither compiler seems to provide us a way to construct a
warning about that.

Would you have any general advice for dealing with this in contrib
software, such as the openzfs example?  What I ended up doing was
disabling the warning for code that uses OPENZFS_CFLAGS.

>> I don't want to overstate the problem though.  As I said, these are
>> annoyances, the solutions aren't hard.
>
>
> They are annoyances that prevent worse problems.
>
>> >>
>> >>  - Wredundant-decls, I'm not sure about.  I have never seen this detect
>> >>    anything that will cause misbehavior, but most of the time that it
>> >>    fires it does indicate some kind of genuine--but harmless--mistake.
>> >
>> >
>> > It also tends to prevent declarations that are the same on some platforms, but different on others.
>> >
>> >>
>> >>  - Wmissing-include-dirs doesn't seem to occur often and usually
>> >>    indicates a genuine (but again harmless) mistake in a makefile.  I
>> >>    think we should keep it.
>> >
>> >
>> > I do too.
>> >
>> >>
>> >>  - Wempty-body, Wunused-function.  I'm not sure.  These are proscriptive
>> >>    about things that are not necessarily problems.  We are apparently
>> >>    already clean for them in the kernel gcc build, so perhaps we should
>> >>    enable them for the kernel clang build.  In any case, I think we
>> >>    should bring these into agreement between clang and gcc, one way or
>> >>    the other.
>> >
>> >
>> > We do not remove it. We include the warning, but don't make it fatal for clang.
>> >
>>
>> Yes, "clean" was maybe not the right word.  The end result is build
>> errors with gcc, no build errors with clang.  The suggestion is just
>> that we do the same thing with both compilers, whether that be an error,
>> or a warning without an error.
>
>
> We should clean that part up. I think we just disagree over how to do that.
>
>> > Most of the clang neutering was due to bugs in the early days of clang. I'm not sure anybody has systematically reviewed them since then.
>> >
>> >> Another sticking point may be contrib software.  I think we generally
>> >> don't want to fail builds of contrib software for things that are
>> >> ultimately harmless.  For bsd.sys.mk this could be accomplished by
>> >> enabling such warnings only at WARNS >= 6.  For the kernel, we could
>> >> come up with some other mechanism.
>> >
>> >
>> > The kernel is different. 'Ultimately harmless' is a value judgement.
>> >
>>
>> I largely agree.  What I'm hoping to break out of is situations where we
>> import contrib software and it breaks the gcc build for reasons that
>> don't affect the functionality of the built code, and then it's a pain
>> to fix because it's contrib software.
>
>
> Yes. I agree the differences are a problem and need a solution. I'm not sure I agree with the specific suggestions is all.
>
> Warner
>

Great.  I am not attached to the specific suggestions.

>> >>
>> >> I'll put up a review soon for deleting -Wnested-externs unless there are
>> >> objections.  If there is agreement about the others, I'll include those
>> >> too.
>> >>
>> >> Ryan
>> >>
>> >> [1] https://clang.llvm.org/docs/DiagnosticsReference.html
>> >> [2] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
>> >> _______________________________________________
>> >> freebsd-hackers at freebsd.org mailing list
>> >> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>> >> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"