git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>
Subject: [PATCH v2 2/2] config.mak.dev: build with -fno-common
Date: Thu, 15 Oct 2020 15:30:29 -0400	[thread overview]
Message-ID: <20201015193029.GB1129761@coredump.intra.peff.net> (raw)
In-Reply-To: <20201015192845.GD1108210@coredump.intra.peff.net>

It's an easy mistake to define a variable in a header with "int x;" when
you really meant to only declare the variable as "extern int x;"
instead. Clang and gcc will both allow this when building with
"-fcommon"; they put these "tentative definitions" in a common block
which the linker is able to resolve.

This is the default in clang and was the default in gcc until gcc-10,
since it helps some legacy code. However, we would prefer not to rely on
this because:

  - using "extern" makes the intent more clear (so it's a style issue,
    but it's one the compiler can help us catch)

  - according to the gcc manpage, it may yield a speed and code size
    penalty

So let's build explicitly with -fno-common when the DEVELOPER knob is
set, which will let developers using clang and older versions of gcc
notice these problems.

I didn't bother making this conditional on a particular version of gcc.
As far as I know, this option has been available forever in both gcc and
clang, so old versions don't need to avoid it. And we already expect gcc
and clang options throughout config.mak.dev, so it's unlikely anybody
setting the DEVELOPER knob is using anything else. It's a noop on
gcc-10, of course, but it's not worth trying to exclude it there.

Note that there's nothing to fix in the code; we already don't have any
issues here. But if you want to test the patch, you can add a bare "int
x;" into cache.h, which will cause the link step to fail.

Signed-off-by: Jeff King <peff@peff.net>
---
New in this version. It could be queued separately from the first.

 config.mak.dev | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.dev b/config.mak.dev
index 89b218d11a..89fefacd94 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -15,6 +15,7 @@ DEVELOPER_CFLAGS += -Wpointer-arith
 DEVELOPER_CFLAGS += -Wstrict-prototypes
 DEVELOPER_CFLAGS += -Wunused
 DEVELOPER_CFLAGS += -Wvla
+DEVELOPER_CFLAGS += -fno-common
 
 ifndef COMPILER_FEATURES
 COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
-- 
2.29.0.rc1.562.g7bd9bc8902

      parent reply	other threads:[~2020-10-15 19:30 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-08 15:27 [PATCH] clean up extern decl of functions Junio C Hamano
2020-10-09  1:55 ` Denton Liu
2020-10-09  2:47   ` Junio C Hamano
2020-10-09 19:26     ` Junio C Hamano
2020-10-09 19:27       ` [RFC] CodingGuidelines: mark external declarations with "extern" Junio C Hamano
2020-10-09 19:57         ` Jeff King
2020-10-09 20:33           ` Junio C Hamano
2020-10-09 23:00             ` Denton Liu
2020-10-09 23:05               ` Junio C Hamano
2020-10-10  0:37             ` Đoàn Trần Công Danh
2020-10-15  1:36             ` Jeff King
2020-10-15 17:15               ` Junio C Hamano
2020-10-15 19:28                 ` Jeff King
2020-10-15 19:30                   ` [PATCH v2 1/2] usage: define a type for a reporting function Jeff King
2020-10-15 19:30                   ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201015193029.GB1129761@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).