Page MenuHomePhabricator

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

Authored by SjoerdMeijer on Feb 24 2020, 8:18 AM.

Details

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:

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Feb 24 2020, 8:18 AM
SjoerdMeijer updated this revision to Diff 246457.EditedFeb 25 2020, 7:40 AM
SjoerdMeijer retitled this revision from [ARM][AArch64] Default to -fno-common to [Driver] Default to -fno-common.
SjoerdMeijer edited the summary of this revision. (Show Details)
SjoerdMeijer added reviewers: tstellar, jyknight.

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

TODO: doc changes.

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

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

MaskRay added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5664

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 added inline comments.Feb 25 2020, 9:36 AM
clang/test/Driver/no-common.c
2

Just %clang -target %itanium_abi_triple

Delete all the targets.

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

minor test update

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

MaskRay added inline comments.Feb 26 2020, 10:13 AM
clang/test/Driver/clang_f_opts.c
15

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 retitled this revision from [Driver] Default to -fno-common to [Driver] Default to -fno-common for all targets.
SjoerdMeijer edited the summary of this revision. (Show Details)

Removed the CHECK-NOTs from some tests as suggested.

And as also suggested, made the title more descriptive.

Thanks for all suggestions!

MaskRay accepted this revision.Feb 26 2020, 12:23 PM

LGTM. Hope @tstellar and @jyknight can confirm.

This revision is now accepted and ready to land.Feb 26 2020, 12:23 PM

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

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

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

clang/docs/ReleaseNotes.rst
87

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.

clang/docs/ReleaseNotes.rst
87

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 updated this revision to Diff 246917.EditedFeb 27 2020, 5:13 AM
SjoerdMeijer edited the summary of this revision. (Show Details)

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.

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

tstellar accepted this revision.Mar 2 2020, 7:36 AM

LGTM.

jyknight accepted this revision.Mar 2 2020, 8:31 AM

Looks good modulo minor comments.

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

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

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

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

MaskRay accepted this revision.Mar 2 2020, 9:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 1:43 AM

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)

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.

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.