Date: Wed, 6 Jan 2021 13:55:40 -0800 From: Ryan Libby <rlibby@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: FreeBSD Hackers <freebsd-hackers@freebsd.org> Subject: Re: clang vs gcc warning flags Message-ID: <CAHgpiFzAxkLu44wZ5Qjid_ePsv63PCNo=omXZkH2xRggn4ho2Q@mail.gmail.com> In-Reply-To: <CANCZdfo4Os96yVjyaYxH5yZW%2Bq9akf6LuP7uR-ck%2B%2BvaiA1czg@mail.gmail.com> References: <CAHgpiFwK77o3J6Bm_3GAQRGAJz70=n8Z8bqqLirahL_gqXCM3w@mail.gmail.com> <CANCZdfpHC6ovmP9syA=8V-92AjdYyRngO0o8m44JwqEtMkNzDA@mail.gmail.com> <CAHgpiFw7rsqAQ-M%2BnsXzvFCLPgEasqnMjkEyMjw1zaAAgQrsGA@mail.gmail.com> <CANCZdfo4Os96yVjyaYxH5yZW%2Bq9akf6LuP7uR-ck%2B%2BvaiA1czg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 6, 2021 at 1:43 PM Warner Losh <imp@bsdimp.com> wrote:
>
>
>
> On Wed, Jan 6, 2021, 2:34 PM Ryan Libby <rlibby@freebsd.org> wrote:
>>
>> On Wed, Jan 6, 2021 at 12:38 PM Warner Losh <imp@bsdimp.com> wrote:
>> >
>> >
>> >
>> > On Wed, Jan 6, 2021 at 12:53 PM Ryan Libby <rlibby@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 suc=
h
>> >> flags [1][2], and a few of them are in our default set of warnings, s=
uch
>> >> 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 witho=
ut
>> >> value. I suggest we just delete it.
>> >
>> >
>> >
>> > It's to prevent declaring functions inside of functions. why is that b=
ad? It only has scope of the function, and we've decided that's not somethi=
ng we want in the tree... I'd say that the fact we don't have any in the tr=
ee 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. Functi=
ons should be declared in headers, lest they be declared differently in dif=
ferent places. That is the harm this guards against. That is far from benig=
n.
>
>> 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=3Dd576ccdf01a4c6f8f02e8ed7e72290=
c729d68de6
>> https://cgit.freebsd.org/src/tree/sys/contrib/openzfs/module/zfs/arc.c#n=
4721
>>
>> 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 dete=
ct
>> >> 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 platfo=
rms, 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 proscript=
ive
>> >> about things that are not necessarily problems. We are apparently
>> >> already clean for them in the kernel gcc build, so perhaps we shou=
ld
>> >> enable them for the kernel clang build. In any case, I think we
>> >> should bring these into agreement between clang and gcc, one way o=
r
>> >> the other.
>> >
>> >
>> > We do not remove it. We include the warning, but don't make it fatal f=
or 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 tha=
t.
>
>> > 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 >=3D 6. For the kernel, we coul=
d
>> >> 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 s=
ure 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 tho=
se
>> >> too.
>> >>
>> >> Ryan
>> >>
>> >> [1] https://clang.llvm.org/docs/DiagnosticsReference.html
>> >> [2] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
>> >> _______________________________________________
>> >> freebsd-hackers@freebsd.org mailing list
>> >> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>> >> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd=
.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHgpiFzAxkLu44wZ5Qjid_ePsv63PCNo=omXZkH2xRggn4ho2Q>