From owner-freebsd-toolchain@FreeBSD.ORG Mon Apr 15 19:26:38 2013 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 469A98F2 for ; Mon, 15 Apr 2013 19:26:38 +0000 (UTC) (envelope-from edschouten@gmail.com) Received: from mail-vb0-x22b.google.com (mail-vb0-x22b.google.com [IPv6:2607:f8b0:400c:c02::22b]) by mx1.freebsd.org (Postfix) with ESMTP id 0E6CE15C3 for ; Mon, 15 Apr 2013 19:26:37 +0000 (UTC) Received: by mail-vb0-f43.google.com with SMTP id q12so4048021vbe.16 for ; Mon, 15 Apr 2013 12:26:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:sender:date:x-google-sender-auth:message-id :subject:from:to:content-type; bh=0FrZMd1MdpYH6WOEAM1CGo5QYFl0OYVfhnRJpmt5wGk=; b=sBkCcNEipJrcqifZGXFgbcLyDTDOrmolVlcbHtdhtDEk17OD8rbQODxA9PPjEpa6rA ED3qn0phRSJ8sHhzWIY7ltQRQ3irSuj7pgUfde3i/ki0yhc0JF4R1si31RBDcRzehnjm oKDrrDJJlipvZoaE+INBvKmrsEMRr2iZbSBSEL/X8JebUTmX11xZyQQ4JOxUtDyV9Utj 2HIXUcLKVZCqEZ2fbImcAm2Y23zDMF/YjzLWBIt/5kiZn8vqgQvR+SHsFc7o0sggliPf oHI54JyzRtfnCKVWTniVV6X8KIbZ6L1iIDctFGrvsOjhFzb3U7pOcY2CT7YSeVPcXvXB wynQ== MIME-Version: 1.0 X-Received: by 10.52.65.113 with SMTP id w17mr5291361vds.79.1366053680986; Mon, 15 Apr 2013 12:21:20 -0700 (PDT) Sender: edschouten@gmail.com Received: by 10.220.102.6 with HTTP; Mon, 15 Apr 2013 12:21:20 -0700 (PDT) Date: Mon, 15 Apr 2013 21:21:20 +0200 X-Google-Sender-Auth: VKcG2JqV-pXvdASpVtua0Sl7zhk Message-ID: Subject: [Patch] Add -Wmissing-variable-declarations to WARNS=6 From: Ed Schouten To: freebsd-toolchain@freebsd.org Content-Type: text/plain; charset=UTF-8 X-BeenThere: freebsd-toolchain@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Maintenance of FreeBSD's integrated toolchain List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Apr 2013 19:26:38 -0000 Hi folks, Quite some time ago I spent some time hacking on Clang to add support for a new warning flag, called -Wmissing-variable-declarations. To summarize, it lets the compiler trigger a warning if you write the following piece of code (at the global scope): int i; Huh? Why should it do that? Well, people should typically write the following instead: extern int i; ... int i; Or: static int i; Essentially, it is the same as -Wmissing-prototypes, but then for global variables. By enabling this compiler warning, I managed to improve our code quality a bit: - In some cases binaries became smaller as I added static keywords, as either the number of symbols in the header decreased, or the compiler could do some magic optimisation that was otherwise not possible. Also in many cases, I observed that the compiler can now more accurately derive which variables are not written to, meaning they are stored in read-only sections. - In some cases I found truly harmful bugs. For example, multiple C source files in one binary would do a tentative definition of a variable under the same name, which got shared, even though they had to remain separated. - In other cases I found harmless bugs. For example: enum { FOO, BAR } baz; Where the following was meant: enum baz { FOO, BAR }; Or simply: enum { FOO, BAR }; Because I think it's a nice compiler warning flag, I am thinking about adding it to WARNS=6. Patch: http://80386.nl/pub/wmissing-variable-declarations.txt I thought I'd send this email, because I've got a couple of questions: - Essentially any piece of code that uses Yacc/Lex won't build because of this flag, simply because the C code that's printed out by these tools is too horrible and cannot easily be fixed without changing the consumers as well. To work around this, I've added a NO_WMISSING_VARIABLE_DECLARATIONS=. Any objections? - Is the logic I added to share/mk/bsd.sys.mk all right? Do I check the right conditions? - Is it okay if I push in the changes to the CDDL code? I already sent a patch for this to the Illumos folks, but so far I haven't received a reply yet: https://www.illumos.org/issues/3700 Enabling this for the kernel would be awesome, but unfortunately this would be pretty hard. We quite often have that we create non-static global variables that are used elsewhere (e.g. through symbol lookups or by placing them in different sections). Cheers, -- Ed Schouten