From owner-freebsd-hackers@freebsd.org Wed Jan 6 21:55:53 2021 Return-Path: Delivered-To: freebsd-hackers@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 136B44E099F for ; Wed, 6 Jan 2021 21:55:53 +0000 (UTC) (envelope-from rlibby@gmail.com) Received: from mail-qv1-f42.google.com (mail-qv1-f42.google.com [209.85.219.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DB38S70GDz3MSG for ; Wed, 6 Jan 2021 21:55:52 +0000 (UTC) (envelope-from rlibby@gmail.com) Received: by mail-qv1-f42.google.com with SMTP id j18so1971849qvu.3 for ; Wed, 06 Jan 2021 13:55:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=9ecAUeO9ppXptFMog4j9w2kp4JhgUAHxNWtHmExFa0I=; b=fbaO6lpAH7kZDutmTc55tzkzkOlLWM1CgIIECQgacBmTUbetMPJmWGlYdiLfGxdYPA kc/5mVSMgRA5fttJ4I7QSgGrObEyHLvQb63nUEpktqhmVCj6kdZIi69QjfV2UWa2msqS vbinCJvxYmRcmRAgC5+GLGvI/LoD+1vMLojDJMm3xGcCKkx9ZRs7IgdaZvZJG9SmdTTG lll66V99QtA+XReYbtmiX25hxgFTNYARlMIIYZNJZ9fjB1XxkYkFcnmk91GvFjOrhw2r PNlNoJQH0c/8FFzEtCuhYsrNQn8AiK2a/sXodqspnLoGKSHIwho1EwKpFzUPtsvv4EBH GW9w== X-Gm-Message-State: AOAM53285QdlTSaI7QZ4Pn+dq2n+gXphlz1UaONyGevwY9e2ZVY2ynWL /u5ZNDqCOdURwQ2p5RwAKcracbU6P3w= X-Google-Smtp-Source: ABdhPJzMvYcQxogN+Ll7opYujD85i22APdUjriSaKLkws885sRpwkK4W8JFrcZ7/4asVoGGLz23DIQ== X-Received: by 2002:a0c:a366:: with SMTP id u93mr6115369qvu.53.1609970151713; Wed, 06 Jan 2021 13:55:51 -0800 (PST) Received: from mail-qk1-f181.google.com (mail-qk1-f181.google.com. [209.85.222.181]) by smtp.gmail.com with ESMTPSA id c7sm2222648qkm.99.2021.01.06.13.55.51 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 06 Jan 2021 13:55:51 -0800 (PST) Received: by mail-qk1-f181.google.com with SMTP id 19so3935701qkm.8 for ; Wed, 06 Jan 2021 13:55:51 -0800 (PST) X-Received: by 2002:a05:620a:12e5:: with SMTP id f5mr6272211qkl.331.1609970151175; Wed, 06 Jan 2021 13:55:51 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Ryan Libby Date: Wed, 6 Jan 2021 13:55:40 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: clang vs gcc warning flags To: Warner Losh Cc: FreeBSD Hackers Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4DB38S70GDz3MSG X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jan 2021 21:55:53 -0000 On Wed, Jan 6, 2021 at 1:43 PM Warner Losh wrote: > > > > On Wed, Jan 6, 2021, 2:34 PM Ryan Libby wrote: >> >> On Wed, Jan 6, 2021 at 12:38 PM Warner Losh wrote: >> > >> > >> > >> > On Wed, Jan 6, 2021 at 12:53 PM Ryan Libby 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"