[Driver] Default to -fno-common for all targets

[SjoerdMeijer]

Summary

This makes -fno-common the default for all targets because this has performance and code-size benefits and is more language conforming for C code. Additionally, GCC10 also defaults to -fno-common and so we get consistent behaviour with GCC.

With this change, C code that uses tentative definitions as definitions of a variable in multiple translation units will trigger multiple-definition linker errors. Generally, this occurs when the use of the extern keyword is neglected in the declaration of a variable in a header file. In some cases, no specific translation unit provides a definition of the variable. The previous behavior can be restored by specifying -fcommon.

As GCC has switched already, we benefit from applications already being ported and existing documentation how to do this. For example:

• https://gcc.gnu.org/gcc-10/porting_to.html
• https://wiki.gentoo.org/wiki/Gcc_10_porting_notes/fno_common

[SjoerdMeijer]

Following the discussion on the cfe dev list, this is a first draft to enable -fno-common for all targets.

TODO: doc changes.

[tstellar]

I am in favor of this change. Could you also add something to the release notes?

[SjoerdMeijer]

Thanks, have added a note to the release notes, and also to the command line reference.

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

5651

cc1 does not check -fcommon: Opts.NoCommon = Args.hasArg(OPT_fno_common);

I think you can flip it to Opts.NoCommon = !Args.hasArg(OPT_fcommon);

and change here to pass -fcommon if Args.hasFlag(options::OPT_fcommon, options::OPT_fno_common, false)

[MaskRay]

clang/test/Driver/no-common.c

2

Just %clang -target %itanium_abi_triple

Delete all the targets.

[SjoerdMeijer]

Thanks for catching that. Now it shows more changes in tests, so updated a bunch of tests.

[SjoerdMeijer]

minor test update

[hubert.reinterpretcast]

I have no objections; however, it may help for the title of the patch to clearly indicate "for all targets".

[MaskRay]

clang/test/Driver/clang_f_opts.c

15 ↗

(On Diff #246770)

I think such NOT patterns should just be deleted.

(Note that NOT patterns tend to be insufficient due to order reasons)

-fcommon is a minority now. We only need to test it for an OS/Arch that really needs it (currently no).

[SjoerdMeijer]

Removed the CHECK-NOTs from some tests as suggested.

And as also suggested, made the title more descriptive.

Thanks for all suggestions!

[MaskRay]

LGTM. Hope @tstellar and @jyknight can confirm.

[jyknight]

Are there any tests remaining that check that with -fcommon, IR generation creates "common" variables, now that all these tests have been modified? If there are not, one should be added.

clang/docs/ClangCommandLineReference.rst

1311 ↗

(On Diff #246808)

Use "instead of generating" instead of "or generate".

Mention "This option has no effect in C++ mode."

clang/docs/ReleaseNotes.rst

87 ↗

(On Diff #246808)

Might be nice to expand upon this somewhat, to give users a clue what it means to them.

E.g.

Therefore, C code which uses multiple definitions of a global variable will trigger a multiple-definition linker errors. Generally this occurs when a variable in a header file neglects to use the "extern" keyword on the declaration. The previous behavior can be restored by specifying -fcommon.

[hubert.reinterpretcast]

clang/docs/ReleaseNotes.rst

87 ↗

(On Diff #246808)

Suggestion:
Therefore, C code that uses tentative definitions as definitions of a variable in multiple translation units will trigger multiple-definition linker errors. Generally, this occurs when the use of the extern keyword is neglected in the declaration of a variable in a header file. In some cases, no specific translation unit provides a definition of the variable. The previous behavior can be restored by specifying -fcommon.

[SjoerdMeijer]

Are there any tests remaining that check that with -fcommon, IR generation creates "common" variables, now that all these tests have been modified?

I've added a RUN line to clang/test/CodeGen/no-common.c with -fcommon that should test this.

Suggestion:

Therefore, C code that uses tentative definitions as definitions of a variable in multiple translation units will trigger multiple-definition linker errors. Generally, this occurs when the use of the extern keyword is neglected in the declaration of a variable in a header file. In some cases, no specific translation unit provides a definition of the variable. The previous behavior can be restored by specifying -fcommon.

Thanks for your suggestion. I've verbatim added this to the release note.

[SjoerdMeijer]

Friendly ping, and just checking: are we happy with this? Good to go?

[tstellar]

LGTM.

[jyknight]

Looks good modulo minor comments.

clang/test/CodeGen/microsoft-no-common-align.c

1 ↗

(On Diff #246917)

I think this test should specify -fcommon, since it's explicitly testing a bug we had in common support?

clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

182–184 ↗

(On Diff #246917)

Sounds like this test is also expecting to check -fcommon behavior.

[SjoerdMeijer]

Thanks all for the reviews and help.
I will fix these things and do a last rebase/check before committing this tomorrow.

[SjoerdMeijer]

Reverted in: https://reviews.llvm.org/rG4e363563fa14

Put up for review some fixes for compiler-rt tests in: https://reviews.llvm.org/D75520

Now checking what these test-suite failures are about:

FAIL: MultiSource/Applications/JM/ldecod/ldecod.compile_time (1 of 2630)
FAIL: MultiSource/Applications/JM/lencod/lencod.compile_time (2 of 2630)
FAIL: MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk.compile_time (3 of 2630)
FAIL: MultiSource/Benchmarks/DOE-ProxyApps-C/miniAMR/miniAMR.compile_time (4 of 2630)
FAIL: MultiSource/Benchmarks/Olden/bh/bh.compile_time (5 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/TimberWolfMC/timberwolfmc.compile_time (6 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/compiler/compiler.compile_time (7 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/loader/loader.compile_time (8 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/simulator/simulator.compile_time (9 of 2630)
FAIL: MultiSource/Applications/JM/ldecod/ldecod.execution_time (527 of 2630)
FAIL: MultiSource/Applications/JM/lencod/lencod.execution_time (528 of 2630)
FAIL: MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk.execution_time (529 of 2630)
FAIL: MultiSource/Benchmarks/DOE-ProxyApps-C/miniAMR/miniAMR.execution_time (530 of 2630)
FAIL: MultiSource/Benchmarks/Olden/bh/bh.execution_time (531 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/TimberWolfMC/timberwolfmc.execution_time (532 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/compiler/compiler.execution_time (533 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/loader/loader.execution_time (534 of 2630)
FAIL: MultiSource/Benchmarks/Prolangs-C/simulator/simulator.execution_time (535 of 2630)

[SjoerdMeijer]

Ha, these test-suite apps fail with multiple definition of ... link errors, so actually require a little bit of porting! :-)
But I think I will prepare a patch that adds -fcommon to their makefiles, as I don't want to change too many things at the same time.

[MaskRay]

In D75056#1903363, @SjoerdMeijer wrote:

Ha, these test-suite apps fail with multiple definition of ... link errors, so actually require a little bit of porting! :-)
But I think I will prepare a patch that adds -fcommon to their makefiles, as I don't want to change too many things at the same time.

Adding -fcommon looks good.

[SjoerdMeijer]

Patch for the test-suite: D75557

clang/lib/Driver/ToolChains/Clang.cpp

static bool isSignedCharDefault(const llvm::Triple &Triple) {
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64:
   case llvm::Triple::systemz:
   case llvm::Triple::xcore:
     return false;
   }
 }
 
-static bool isNoCommonDefault(const llvm::Triple &Triple) {
-  switch (Triple.getArch()) {
-  default:
-    if (Triple.isOSFuchsia())
-      return true;
-    return false;
-
-  case llvm::Triple::xcore:
-  case llvm::Triple::wasm32:
-  case llvm::Triple::wasm64:
-    return true;
-  }
-}
-
 static bool hasMultipleInvocations(const llvm::Triple &Triple,
                                    const ArgList &Args) {
   // Supported only on Darwin where we invoke the compiler multiple times
   // followed by an invocation to lipo.
   if (!Triple.isOSDarwin())
     return false;
   // If more than one "-arch <arch>" is specified, we're targeting multiple
   // architectures resulting in a fat binary.
if (!SkipMaxTypeAlign) {
       std::string MaxTypeAlignStr = "-fmax-type-align=16";
       CmdArgs.push_back(Args.MakeArgString(MaxTypeAlignStr));
     }
   }
 
   if (!Args.hasFlag(options::OPT_Qy, options::OPT_Qn, true))
     CmdArgs.push_back("-Qn");
 
-  // -fcommon is the default unless compiling kernel code or the target says so
-  bool NoCommonDefault = KernelOrKext || isNoCommonDefault(RawTriple);
-  if (!Args.hasFlag(options::OPT_fcommon, options::OPT_fno_common,
-                    !NoCommonDefault))
+  // -fno-common is the default, set -fcommon only when that flag is set.
+  if (Args.hasFlag(options::OPT_fcommon, options::OPT_fno_common, false))
+    CmdArgs.push_back("-fcommon");
+  else
     CmdArgs.push_back("-fno-common");
 
   // -fsigned-bitfields is default, and clang doesn't yet support
   // -funsigned-bitfields.
   if (!Args.hasFlag(options::OPT_fsigned_bitfields,
                     options::OPT_funsigned_bitfields))
     D.Diag(diag::warn_drv_clang_unsupported)
         << Args.getLastArg(options::OPT_funsigned_bitfields)->getAsString(Args);

clang/test/CodeGen/2009-10-20-GlobalDebug.c

 // REQUIRES: x86-registered-target
 // RUN: %clang -target i386-apple-darwin10 -flto -S -g %s -o - | FileCheck %s
 
 // CHECK: @main.localstatic = internal global i32 0, align 4, !dbg [[L:![0-9]+]]
-// CHECK: @global = common global i32 0, align 4, !dbg [[G:![0-9]+]]
+// CHECK: @global = global i32 0, align 4, !dbg [[G:![0-9]+]]
 
 int global;
-int main() { 
+int main() {
   static int localstatic;
   return 0;
 }
 
 // CHECK: [[L]] = !DIGlobalVariableExpression(var: [[LV:.*]], expr: !DIExpression())
 // CHECK: [[LV]] = distinct !DIGlobalVariable(name: "localstatic"
 // CHECK-NOT:                                 linkageName:
 // CHECK-SAME:                                line: 9,
 // CHECK: [[G]] = !DIGlobalVariableExpression(var: [[GV:.*]], expr: !DIExpression())
 // CHECK: [[GV]] = distinct !DIGlobalVariable(name: "global"
 // CHECK-NOT:                                 linkageName:
 // CHECK-SAME:                                line: 7,

clang/test/Driver/no-common.c

+// RUN: %clang --target=armv7-arm-none-eabi -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=armv7-arm-none-eabi -mbig-endian -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=armebv7-arm-none-eabi -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=thumbv8a-arm-none-eabi -### -c %s 2>&1  | FileCheck %s
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -mbig-endian -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=thumbv8a-arm-none-eabi -mbig-endian -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=thumbebv8a-arm-none-eabi -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=aarch64_be-arm-none-eabi -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=armv8a-arm-none-eabi -fcommon -fno-common -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=mips32 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=mips32el -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=mips64 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=mips64el -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=powerpc -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=powerpc64 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=powerpc64le -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=riscv32 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=riscv64 -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=sparc -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=sparcel -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=sparcv9 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=sparc64 -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=wasm32-unknown -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=wasm64-unknown -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=x86-none -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang --target=x86_64-none -### -c %s 2>&1 | FileCheck %s
+
+// RUN: %clang --target=xcore-unknown -### -c %s 2>&1 | FileCheck %s
+
+// CHECK:     "-fno-common"
+// CHECK-NOT: "-fcommon"

clang/test/Driver/rewrite-legacy-objc.m

 // RUN: %clang -no-canonical-prefixes -target x86_64-apple-macosx10.7.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST0 %s
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
 // TEST0: rewrite-legacy-objc.m"
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.9.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST1 %s
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.6.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST2 %s
-// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
-// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
+// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"

clang/test/Driver/rewrite-objc.m

 // RUN: %clang -no-canonical-prefixes -target x86_64-apple-macosx10.7.0 -rewrite-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST0 %s
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"

clang/test/Frontend/ast-codegen.c

 // REQUIRES: x86-registered-target
 // RUN: %clang -target i386-unknown-unknown -emit-ast -o %t.ast %s
 // RUN: %clang -target i386-unknown-unknown -emit-llvm -S -o - %t.ast | FileCheck %s
 
 // CHECK: module asm "foo"
 __asm__("foo");
 
-// CHECK: @g0 = common dso_local global i32 0, align 4
+// CHECK: @g0 = dso_local global i32 0, align 4
 int g0;
 
 // CHECK: define dso_local i32 @f0()
 int f0() {
 }