[PATCH] D75056: [Driver] Default to -fno-common

SjoerdMeijer updated this revision to Diff 246457. SjoerdMeijer retitled this revision from "[ARM][AArch64] Default to -fno-common" to "[Driver] Default to -fno-common". SjoerdMeijer edited the summary of this revision. SjoerdMeijer added reviewers: tstellar, jyknight. SjoerdMeijer added a comment

[PATCH] D75056: [Driver] Default to -fno-common

tstellar added a comment. I am in favor of this change. Could you also add something to the release notes? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D75056: [Driver] Default to -fno-common

SjoerdMeijer updated this revision to Diff 246491. SjoerdMeijer added a comment. Thanks, have added a note to the release notes, and also to the command line reference. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 Files: clang/docs/ClangCo

[PATCH] D75056: [Driver] Default to -fno-common

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5651 + // -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");

[PATCH] D75056: [Driver] Default to -fno-common

MaskRay added inline comments. Comment at: clang/test/Driver/no-common.c:1 +// 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 Just `%clang -target %

[PATCH] D75056: [Driver] Default to -fno-common

SjoerdMeijer updated this revision to Diff 246770. SjoerdMeijer added a comment. minor test update CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 Files: clang/docs/ClangCommandLineReference.rst clang/docs/ReleaseNotes.rst clang/include/cl

[PATCH] D75056: [Driver] Default to -fno-common

SjoerdMeijer updated this revision to Diff 246768. SjoerdMeijer added a comment. Herald added subscribers: kerbowa, nhaehnle, jvesely. Thanks for catching that. Now it shows more changes in tests, so updated a bunch of tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ htt

[PATCH] D75056: [Driver] Default to -fno-common

hubert.reinterpretcast added a comment. I have no objections; however, it may help for the title of the patch to clearly indicate "for all targets". CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 _

[PATCH] D75056: [Driver] Default to -fno-common

MaskRay added inline comments. Comment at: clang/test/Driver/clang_f_opts.c:15 // CHECK-OPTIONS2: -fshort-enums -// CHECK-OPTIONS2: -fno-common +// CHECK-OPTIONS2-NOT: -fcommon // CHECK-OPTIONS2: -fno-show-source-location I think such NOT patterns should just b

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer updated this revision to Diff 246808. 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. SjoerdMeijer added a comment. Removed the CHECK-NOTs from some test

[PATCH] D75056: [Driver] Default to -fno-common for all targets

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Hope @tstellar and @jyknight can confirm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 _

[PATCH] D75056: [Driver] Default to -fno-common for all targets

jyknight added a comment. 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. Comment at: clang/docs/ClangCommandLineReference.rst:1311 +Plac

[PATCH] D75056: [Driver] Default to -fno-common for all targets

hubert.reinterpretcast added inline comments. Comment at: clang/docs/ReleaseNotes.rst:87 +- -fno-common has been enabled as the default for all targets. jyknight wrote: > Might be nice to expand upon this somewhat, to give users a clue what it > means to the

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer updated this revision to Diff 246917. SjoerdMeijer edited the summary of this revision. SjoerdMeijer added a comment. > 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

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer added a comment. Friendly ping, and just checking: are we happy with this? Good to go? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D75056: [Driver] Default to -fno-common for all targets

tstellar accepted this revision. tstellar added a comment. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin

[PATCH] D75056: [Driver] Default to -fno-common for all targets

jyknight accepted this revision. jyknight added a comment. Looks good modulo minor comments. Comment at: clang/test/CodeGen/microsoft-no-common-align.c:1 // RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -o - %s | FileCheck %s typedef float TooLargeAlignment __attrib

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer added a comment. Thanks all for the reviews and help. I will fix these things and do a last rebase/check before committing this tomorrow. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 ___

[PATCH] D75056: [Driver] Default to -fno-common for all targets

This revision was automatically updated to reflect the committed changes. Closed by commit rG0a9fc9233e17: [Driver] Default to -fno-common for all targets (authored by SjoerdMeijer). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D75056?vs=246917&id=247822#toc

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer added a comment. 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

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer added a comment. 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. R

[PATCH] D75056: [Driver] Default to -fno-common for all targets

MaskRay added a comment. 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

[PATCH] D75056: [Driver] Default to -fno-common for all targets

SjoerdMeijer added a comment. Patch for the test-suite: D75557 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75056/new/ https://reviews.llvm.org/D75056 ___ cfe-commits