Re: [PATCH] PR85678: Change default to -fno-common

Re: [PATCH] PR85678: Change default to -fno-common

[David Edelsohn]

>> Has this been bootstrapped and regression tested?
>
> Yes, it bootstraps OK of course. I ran regression over the weekend, there
> are a few minor regressions in lto due to relying on tentative definitions
> and a few latent bugs. I'd expect there will be a few similar failures on
> other targets but nothing major since few testcases rely on -fcommon.

This almost certainly will break AIX.

- David

Re: [PATCH] PR85678: Change default to -fno-common

[Richard Biener]

On October 28, 2019 7:43:33 PM GMT+01:00, David Edelsohn <dje.gcc@gmail.com> wrote:
>>> Has this been bootstrapped and regression tested?
>>
>> Yes, it bootstraps OK of course. I ran regression over the weekend,
>there
>> are a few minor regressions in lto due to relying on tentative
>definitions
>> and a few latent bugs. I'd expect there will be a few similar
>failures on
>> other targets but nothing major since few testcases rely on -fcommon.
>
>This almost certainly will break AIX.

I suppose targets can override this decision. 

Richard. 

>- David

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 10/28/19 1:43 PM, Richard Biener wrote:
> On October 28, 2019 7:43:33 PM GMT+01:00, David Edelsohn <dje.gcc@gmail.com> wrote:
>>>> Has this been bootstrapped and regression tested?
>>>
>>> Yes, it bootstraps OK of course. I ran regression over the weekend,
>> there
>>> are a few minor regressions in lto due to relying on tentative
>> definitions
>>> and a few latent bugs. I'd expect there will be a few similar
>> failures on
>>> other targets but nothing major since few testcases rely on -fcommon.
>>
>> This almost certainly will break AIX.
> 
> I suppose targets can override this decision. 
I think they probably could via the override_options mechanism.

Jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi,

>> I suppose targets can override this decision. 
> I think they probably could via the override_options mechanism.

Yes, it's trivial to add this to target_option_override():

  if (!global_options_set.x_flag_no_common)
    flag_no_common = 0;

Cheers,
Wilco

Re: [PATCH] PR85678: Change default to -fno-common

[Iain Sandoe]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>>> 
>>> I suppose targets can override this decision. 
>> I think they probably could via the override_options mechanism.
> 
> Yes, it's trivial to add this to target_option_override():
> 
>  if (!global_options_set.x_flag_no_common)
>    flag_no_common = 0;

for the record,  Darwin bootstraps OK with the change (which is to be expected,
since the preferred setting for it is -fno-common).

Testsuite fails are order “a few hundred” mostly seem to be related to tree-prof
and vector tests (plus the anticipated scan-asm stuff, where code-gen will have
changed).  I don’t have cycles to analyse the causes right now - but that gives
an idea.

cheers
Iain

Re: [PATCH] PR85678: Change default to -fno-common

[Richard Biener]

On Mon, Oct 28, 2019 at 10:39 PM Iain Sandoe <idsandoe@googlemail.com> wrote:
>
> Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> >>>
> >>> I suppose targets can override this decision.
> >> I think they probably could via the override_options mechanism.
> >
> > Yes, it's trivial to add this to target_option_override():
> >
> >  if (!global_options_set.x_flag_no_common)
> >    flag_no_common = 0;
>
> for the record,  Darwin bootstraps OK with the change (which is to be expected,
> since the preferred setting for it is -fno-common).
>
> Testsuite fails are order “a few hundred” mostly seem to be related to tree-prof
> and vector tests (plus the anticipated scan-asm stuff, where code-gen will have
> changed).  I don’t have cycles to analyse the causes right now - but that gives
> an idea.

Note the vectorizer tests use explicit -fno-common ... (because we
can't re-align
globals if they are COMMON)

Richard.

> cheers
> Iain
>
>
>

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi Iain,

> for the record,  Darwin bootstraps OK with the change (which is to be expected,
> since the preferred setting for it is -fno-common).

That's good to hear.

> Testsuite fails are order “a few hundred” mostly seem to be related to tree-prof
> and vector tests (plus the anticipated scan-asm stuff, where code-gen will have
> changed).  I don’t have cycles to analyse the causes right now - but that gives
> an idea.

Are those tests specific to Power? I got 14 failures in total across the full
C and C++ test suites. Note it's easy to update the default options for a
specific test directory if needed.

Wilco

Re: [PATCH] PR85678: Change default to -fno-common

[Iain Sandoe]

Hi Wilco,

Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

>> Testsuite fails are order “a few hundred” mostly seem to be related to tree-prof
>> and vector tests (plus the anticipated scan-asm stuff, where code-gen will have
>> changed).  I don’t have cycles to analyse the causes right now - but that gives
>> an idea.
> 
> Are those tests specific to Power? I got 14 failures in total across the full
> C and C++ test suites. Note it's easy to update the default options for a
> specific test directory if needed.

No, the test was on x86_64-darwin16 (m32/m64)  and a very rough estimate of the 
number of fails (but definitely in the hundreds, not tens) - I haven’t tried the same on
PPC; the h/w is tied up with gcc-7 backport testing.

the change for *-*-darwin* will be that the ABI mandates common items to be 
indirected, thus defaulting to no-common will have the effect of changing the
generated code - and it’s a fairly common pattern in the testsuite to have uninit globals.

 * I’d expect broken scan-asms (probably not difficult to fix, just tedious).
 * not sure why it seems to have a particularly adverse effect on the tree-prof stuff.

short on cycles in the run up to stage #1 end + gcc-7 release .. but will try to 
poke at it over the next few days.

cheers
Iain

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 12/5/19 6:18 AM, Wilco Dijkstra wrote:
> Hi,
> 
> I have updated the documentation patch here and added relevant maintainers
> so hopefully this can go in soon: https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00311.html
> 
> I moved the paragraph in changes.html to the C section like you suggested. Would
> it make sense to link to the porting_to entry?
It might also be helpful to discuss the motivation behind the change.
We're making work for a lot of people with no obvious benefits.

And it's often more complex than just adding an "extern" in a header
file.  Often there isn't a definition anywhere, so you have to go add it
somewhere.  If the package happens to create DSOs, then you have to add
it to the right place or else the DSOs will have undefined references to
the symbols.  If the package doesn't link against and use the DSO, then
you may not know you put the definition in the wrong place until some
*other* package tries to use the DSO and gets an undefined symbol.

Jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 12/5/19 2:16 AM, Martin Liška wrote:
> 
>>
>> Of the ~450 packages affected I'd estimate that even with the opt-out
>> mechanism we're still going to have to fix ~100 packages immediately
>> because they don't honor the flags injection mechanisms which the
>> opt-out approach relies upon.
> 
> Fortunately, we switch at openSUSE to use -fpie by default and our packages
> honor the flags ;)
:-)  Fedora is in much worse shape than RHEL simply because we haven't
pushed hard on the injection issues for Fedora and because there's a ton
of stuff in Fedora that isn't in RHEL.

So it looks like ~150 packages in Fedora that are affected by the
-fno-common change and which aren't honoring the flags injection.  Not
great, but probably manageable.

> 
> I would like to mention here that key work is to report and explain that
> to upstream. Only that will help for the future to reduce number of
> packages
> that will need the -fcommon option. That's the biggest effort in my
> opinion.
Absolutely.  I've found this takes more time than fixing the issue in
the first place on other stuff and I'd expect it to be no different than
the -fno-common stuff.

jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi,

I have updated the documentation patch here and added relevant maintainers
so hopefully this can go in soon: https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00311.html

I moved the paragraph in changes.html to the C section like you suggested. Would
it make sense to link to the porting_to entry?

Cheers,
Wilco

Re: [PATCH] PR85678: Change default to -fno-common

[Tobias Burnus]

On 12/5/19 10:16 AM, Martin Liška wrote:
> I would like to mention here that key work is to report and explain that
> to upstream. Only that will help for the future to reduce number of 
> packages
> that will need the -fcommon option. That's the biggest effort in my 
> opinion.

For this, it would help to update the porting-to web page. Wilco had a 
first version of such a patch at: 
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02662.html – I think it 
should be reviewed and eventually committed.

(Regarding the changes.html change, I think it should be under "C" and 
not under general improvements; as far as I can see, it only affects C 
(and ObjC?), even though the flag is also used for 
lto_symtab_resolve_replaceable_p and in ada/gcc-interface/utils.c.)

Cheers,

Tobias

Re: [PATCH] PR85678: Change default to -fno-common

[Martin Liška]

On 12/4/19 6:27 PM, Jeff Law wrote:
> On 12/4/19 8:24 AM, Wilco Dijkstra wrote:
>> Hi Jeff,
>>
>>>> I've noticed quite significant package failures caused by the revision.
>>>> Would you please consider documenting this change in porting_to.html
>>>> (and in changes.html) for GCC 10 release?
>>>
>>> I'm not in the office right now, but figured I'd chime in.  I'd estimate
>>> 400-500 packages are failing in Fedora because of this change.  I'll
>>> have a hard number Monday.
>>>
>>> It's significant enough that I'm not sure how we're going to get them
>>> all fixed.
>>
>> So what normally happens with the numerous new warnings/errors in GCC
>> releases? I suppose that could cause package failures too. Would it be feasible
>> to override the options for any failing packages?
> Usually we're talking about a few dozen packages that are tripped by any
> particular issue.  The -fno-common issue is a full order of magnitude
> larger.  My builds show ~450 failures due to this issue.
> 
> We're investigating different approaches that don't just involve
> reverting the patch or reverting for Fedora.  Those just kick the can
> down the road with no real progress and we're in the same position next
> year.
> 
> The approach that seems most feasible would be to have an opt-out
> mechanism.  That would keep -fno-common as the default but provide a
> mechanism for a package to opt-out.

Hello.

I'm going to discuss an approach that will be selected for openSUSE distribution.

> 
> Of the ~450 packages affected I'd estimate that even with the opt-out
> mechanism we're still going to have to fix ~100 packages immediately
> because they don't honor the flags injection mechanisms which the
> opt-out approach relies upon.

Fortunately, we switch at openSUSE to use -fpie by default and our packages
honor the flags ;)

>  I'm going to do some testing
> today/tomorrow to see how many affected packages don't honor the flags
> injection mechanisms we use.
> 
> If indeed it's ~100 packages that don't honor the flags injection, then
> we're looking at adding 350 markers to opt-out plus ~100 real
> fixes/workarounds.  That's still a lot of mindless work, but probably in
> the realm of possible.

I would like to mention here that key work is to report and explain that
to upstream. Only that will help for the future to reduce number of packages
that will need the -fcommon option. That's the biggest effort in my opinion.

> 
> I will highlight that having the tester has proven hugely valuable here.
>   If we'd found out about this later in the process we'd probably be
> looking seriously at reversion.

Fully agree here.
Martin

> 
> jeff
> 

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 12/4/19 2:03 PM, Joseph Myers wrote:
> On Wed, 4 Dec 2019, Jeff Law wrote:
> 
>>> So what normally happens with the numerous new warnings/errors in GCC
>>> releases? I suppose that could cause package failures too. Would it be feasible
>>> to override the options for any failing packages?
>> Usually we're talking about a few dozen packages that are tripped by any
>> particular issue.  The -fno-common issue is a full order of magnitude
>> larger.  My builds show ~450 failures due to this issue.
> 
> I'll note here as an advance warning that WG14 has indicated support for 
> making bool, true and false into keywords for C2x, which seems likely to 
> be the sort of change that would cause large numbers of build failures in 
> pre-C99 code that does "typedef char bool;" and similar.  Assuming it does 
> get into C2x, it might make sense, once implemented in GCC, to try such 
> distribution builds with a local patch to change the default C language to 
> -std=gnu2x, to see the extent of the build failures and possibly get them 
> addressed well before the actual GCC default does change to gnu2x.
Agreed.  In fact Jon and I have discussed similar testing for flipping
the C++ default as well.

Jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Joseph Myers]

On Wed, 4 Dec 2019, Jeff Law wrote:

> > So what normally happens with the numerous new warnings/errors in GCC
> > releases? I suppose that could cause package failures too. Would it be feasible
> > to override the options for any failing packages?
> Usually we're talking about a few dozen packages that are tripped by any
> particular issue.  The -fno-common issue is a full order of magnitude
> larger.  My builds show ~450 failures due to this issue.

I'll note here as an advance warning that WG14 has indicated support for 
making bool, true and false into keywords for C2x, which seems likely to 
be the sort of change that would cause large numbers of build failures in 
pre-C99 code that does "typedef char bool;" and similar.  Assuming it does 
get into C2x, it might make sense, once implemented in GCC, to try such 
distribution builds with a local patch to change the default C language to 
-std=gnu2x, to see the extent of the build failures and possibly get them 
addressed well before the actual GCC default does change to gnu2x.

(The proposal also makes alignas, alignof, static_assert and thread_local 
into keywords, but I'd expect those to cause much smaller numbers of build 
failures than bool, true and false.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 12/4/19 8:24 AM, Wilco Dijkstra wrote:
> Hi Jeff,
> 
>>> I've noticed quite significant package failures caused by the revision.
>>> Would you please consider documenting this change in porting_to.html
>>> (and in changes.html) for GCC 10 release?
>>
>> I'm not in the office right now, but figured I'd chime in.  I'd estimate
>> 400-500 packages are failing in Fedora because of this change.  I'll
>> have a hard number Monday.
>>
>> It's significant enough that I'm not sure how we're going to get them
>> all fixed.
> 
> So what normally happens with the numerous new warnings/errors in GCC
> releases? I suppose that could cause package failures too. Would it be feasible
> to override the options for any failing packages?
Usually we're talking about a few dozen packages that are tripped by any
particular issue.  The -fno-common issue is a full order of magnitude
larger.  My builds show ~450 failures due to this issue.

We're investigating different approaches that don't just involve
reverting the patch or reverting for Fedora.  Those just kick the can
down the road with no real progress and we're in the same position next
year.

The approach that seems most feasible would be to have an opt-out
mechanism.  That would keep -fno-common as the default but provide a
mechanism for a package to opt-out.

Of the ~450 packages affected I'd estimate that even with the opt-out
mechanism we're still going to have to fix ~100 packages immediately
because they don't honor the flags injection mechanisms which the
opt-out approach relies upon.  I'm going to do some testing
today/tomorrow to see how many affected packages don't honor the flags
injection mechanisms we use.

If indeed it's ~100 packages that don't honor the flags injection, then
we're looking at adding 350 markers to opt-out plus ~100 real
fixes/workarounds.  That's still a lot of mindless work, but probably in
the realm of possible.

I will highlight that having the tester has proven hugely valuable here.
 If we'd found out about this later in the process we'd probably be
looking seriously at reversion.

jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi Jeff,

>> I've noticed quite significant package failures caused by the revision.
>> Would you please consider documenting this change in porting_to.html
>> (and in changes.html) for GCC 10 release?
>
> I'm not in the office right now, but figured I'd chime in.  I'd estimate
> 400-500 packages are failing in Fedora because of this change.  I'll
> have a hard number Monday.
>
> It's significant enough that I'm not sure how we're going to get them
> all fixed.

So what normally happens with the numerous new warnings/errors in GCC
releases? I suppose that could cause package failures too. Would it be feasible
to override the options for any failing packages?

Cheers,
Wilco

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 11/29/19 6:12 AM, Martin Liška wrote:
> Hello.
> 
> I've noticed quite significant package failures caused by the revision.
> Would you please consider documenting this change in porting_to.html
> (and in changes.html) for GCC 10 release?
I'm not in the office right now, but figured I'd chime in.  I'd estimate
400-500 packages are failing in Fedora because of this change.  I'll
have a hard number Monday.

It's significant enough that I'm not sure how we're going to get them
all fixed.

jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Martin Liška]

On 11/29/19 3:43 PM, Wilco Dijkstra wrote:
> How significant? Is it mostly the common mistake of forgetting extern?

Likely, I see it in at least 400 packages out of 11000 which we have
in openSUSE:Factory. Plus there are many 'nm -B' configure script
defects:
https://lists.gnu.org/archive/html/bug-autoconf/2019-11/msg00001.html

Thank you for the porting to entry!

Martin

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi Martin,

> I've noticed quite significant package failures caused by the revision.

How significant? Is it mostly the common mistake of forgetting extern?

> Would you please consider documenting this change in porting_to.html
> (and in changes.html) for GCC 10 release?

Sure, I already had a patch for changes.html - I've added an initial porting_to
as well:

[wwwdocs] Document -fcommon default change

Add an entry for the default change. Passes the W3 validator.

--
diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html
index f0f0d312171a54afede176f06ce76f9c8abaebc4..980e4e591781d04aa888ba5988981006bd30dd1f 100644
--- a/htdocs/gcc-10/changes.html
+++ b/htdocs/gcc-10/changes.html
@@ -47,6 +47,13 @@ a work-in-progress.</p>
 
 <!-- .................................................................. -->
 <h2 id="general">General Improvements</h2>
+<p>The following GCC command line options have been introduced or improved.</p>
+<ul>
+  <li>GCC now defaults to <code>-fno-common</code>.  In C, global variables with
+      multiple tentative definitions will result in linker errors.
+      Global variable accesses are also more efficient on various targets.
+  </li>
+</ul>
 
 <p>The following built-in functions have been introduced.</p>
 <ul>
diff --git a/htdocs/gcc-10/porting_to.html b/htdocs/gcc-10/porting_to.html
new file mode 100644
index 0000000000000000000000000000000000000000..2e652f6aa4bd3259a316af0c72ab7eb96bab53b7
--- /dev/null
+++ b/htdocs/gcc-10/porting_to.html
@@ -0,0 +1,65 @@
+<!DOCTYPE html>
+<html lang="en">
+
+<head>
+<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+<title>Porting to GCC 10</title>
+<link rel="stylesheet" type="text/css" href="https://gcc.gnu.org/gcc.css" />
+</head>
+
+<body>
+<h1>Porting to GCC 10</h1>
+
+<p>
+The GCC 10 release series differs from previous GCC releases in
+<a href="changes.html">a number of ways</a>. Some of these are a result
+of bug fixing, and some old behaviors have been intentionally changed
+to support new standards, or relaxed in standards-conforming ways to
+facilitate compilation or run-time performance.
+</p>
+
+<p>
+Some of these changes are user visible and can cause grief when
+porting to GCC 10. This document is an effort to identify common issues
+and provide solutions. Let us know if you have suggestions for improvements!
+</p>
+
+
+<!--
+<h2 id="cpp">Preprocessor issues</h2>
+-->
+
+<h2 id="c">C language issues</h2>
+
+<h3 id="complit">Default to <code>-fno-common</code></h3>
+
+<p>
+  A common mistake in C is omitting <code>extern</code> when declaring a global
+  variable in a header file.  If the header is included by several files it
+  results in multiple definitions of the same variable.  In previous GCC
+  versions this error is ignored.  GCC 10 defaults to <code>-fno-common</code>,
+  which means a linker error will now be reported.
+  To fix this, use <code>extern</code> in header files when declaring global
+  variables, and ensure each global is defined in exactly one C file.
+  As a workaround, legacy C code can be compiled with <code>-fcommon</code>.
+</p>
+  <pre><code>
+      int x;  // tentative definition - avoid in header files
+
+      extern int y;  // correct declaration in a header file
+  </code></pre>
+
+<!--
+<h2 id="cxx">C++ language issues</h2>
+-->
+
+<!--
+<h2 id="fortran">Fortran language issues</h2>
+-->
+ 
+<!--
+<h2 id="links">Links</h2>
+-->
+
+</body>
+</html>

Re: [PATCH] PR85678: Change default to -fno-common

[Martin Liška]

Hello.

I've noticed quite significant package failures caused by the revision.
Would you please consider documenting this change in porting_to.html
(and in changes.html) for GCC 10 release?

Thank you

Re: [PATCH] PR85678: Change default to -fno-common

[Segher Boessenkool]

On Mon, Oct 28, 2019 at 03:05:58PM +0000, Wilco Dijkstra wrote:
> > Has this been bootstrapped and regression tested?
> 
> Yes, it bootstraps OK of course. I ran regression over the weekend, there
> are a few minor regressions in lto due to relying on tentative definitions
> and a few latent bugs. I'd expect there will be a few similar failures on
> other targets but nothing major since few testcases rely on -fcommon.
> The big question is how it affects the distros.

On what targets has it been bootstrapped (and tested)?


Segher

Re: [PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

Hi Jeff,

> Has this been bootstrapped and regression tested?

Yes, it bootstraps OK of course. I ran regression over the weekend, there
are a few minor regressions in lto due to relying on tentative definitions
and a few latent bugs. I'd expect there will be a few similar failures on
other targets but nothing major since few testcases rely on -fcommon.
The big question is how it affects the distros.

Wilco

Re: [PATCH] PR85678: Change default to -fno-common

[Harald van Dijk]

On 25/10/2019 16:47, Wilco Dijkstra wrote:
> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
> C feature which is not conforming with the latest C standards.

The PR references C99/C11 6.9p5, but that is not a constraint. Any 
violation merely renders the behaviour of a program undefined, so no 
diagnostic is required. To the best of my knowledge, both -fcommon and 
-fno-common are compatible with the C standard.

This is no reason not to change the default, but...

>[...]
> +definition.  This behavior does not conform to ISO C, is inconsistent with C++,
> +and on many targets implies a speed and code size penalty on global variable
> +references.  It is mainly useful to enable legacy code to link without errors.

...can this inaccurate characterization be left out of the documentation?

Cheers,
Harald van Dijk

Re: [PATCH] PR85678: Change default to -fno-common

[Jeff Law]

On 10/25/19 9:47 AM, Wilco Dijkstra wrote:
> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
> C feature which is not conforming with the latest C standards.  On many targets
> this means global variable accesses have a codesize and performance penalty.
> This applies to C code only, C++ code is not affected by -fcommon.  It is about
> time to change the default.
> 
> OK for commit?
> 
> ChangeLog
> 2019-10-25  Wilco Dijkstra  <wdijkstr@arm.com>
> 
> 	PR85678
> 	* common.opt (fcommon): Change init to 1.
> 
> doc/
> 	* invoke.texi (-fcommon): Update documentation.
Has this been bootstrapped and regression tested?

jeff

Re: [PATCH] PR85678: Change default to -fno-common

[Segher Boessenkool]

On Sat, Oct 26, 2019 at 12:21:15PM +0100, Iain Sandoe wrote:
> Georg-Johann Lay <gjl@gcc.gnu.org> wrote:
> > Wilco Dijkstra schrieb:
> >> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
> >> C feature which is not conforming with the latest C standards.  On many targets
> >> this means global variable accesses have a codesize and performance penalty.
> >> This applies to C code only, C++ code is not affected by -fcommon.  It is about
> >> time to change the default.
> >> OK for commit?
> > 
> > IIRC using -fno-common might lead to some testsuit fallout because
> > some optimizations / test cases are sensitive to -f[no-]common.
> > So I wonder that no adjustments to test cases are needed?
> 
> Two points here:
> 
> (a) I actually agree with the idea to change the default

I think everyone does, this has been long overdue.

> (b) there will surely be testsuite fallout on Darwin, where common accesses are
>      mandated to be indirect in the ABI, where non-weak .data accesses are not
>     Thus code generated for Darwin will change for any test using variables that
>     are “common” and where the test does not already force -fno-common
> 
>     I’d expect fallout to be quite large - since there are plenty of testcases with uninit
>    gobals.  We might want to make a test-run and see the size of the problem, but
>    preferably once the stage#1 submisison and gcc-7 release cycles are done.

Yeah -- and if fallout is more than just testsuite, should it be postponed
to GCC 11?  And announced now, etc.

But to find out we probably should flip the switch soon, see what happens,
only flip it back if necessary.


Segher

Re: [PATCH] PR85678: Change default to -fno-common

[Iain Sandoe]

Georg-Johann Lay <gjl@gcc.gnu.org> wrote:

> Wilco Dijkstra schrieb:
>> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>> C feature which is not conforming with the latest C standards.  On many targets
>> this means global variable accesses have a codesize and performance penalty.
>> This applies to C code only, C++ code is not affected by -fcommon.  It is about
>> time to change the default.
>> OK for commit?
> 
> IIRC using -fno-common might lead to some testsuit fallout because
> some optimizations / test cases are sensitive to -f[no-]common.
> So I wonder that no adjustments to test cases are needed?

Two points here:

(a) I actually agree with the idea to change the default

(b) there will surely be testsuite fallout on Darwin, where common accesses are
     mandated to be indirect in the ABI, where non-weak .data accesses are not
    Thus code generated for Darwin will change for any test using variables that
    are “common” and where the test does not already force -fno-common

    I’d expect fallout to be quite large - since there are plenty of testcases with uninit
   gobals.  We might want to make a test-run and see the size of the problem, but
   preferably once the stage#1 submisison and gcc-7 release cycles are done.

thanks
Iain

>> ChangeLog
>> 2019-10-25  Wilco Dijkstra  <wdijkstr@arm.com>
>> 	PR85678
>> 	* common.opt (fcommon): Change init to 1.
>> doc/
>> 	* invoke.texi (-fcommon): Update documentation.
>> ---
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index 0195b0cb85a06dd043fd0412b42dfffddfa2495b..b0840f41a5e480f4428bd62724b0dc3d54c68c0b 100644
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -1131,7 +1131,7 @@ Common Report Var(flag_combine_stack_adjustments) Optimization
>> Looks for opportunities to reduce stack adjustments and stack references.
>>  fcommon
>> -Common Report Var(flag_no_common,0)
>> +Common Report Var(flag_no_common,0) Init(1)
>> Put uninitialized globals in the common section.
>>  fcompare-debug
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 857d9692729e503657d0d0f44f1f6252ec90d49a..5b4ff66015f5f94a5bd89e4dc3d2d53553cc091e 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -568,7 +568,7 @@ Objective-C and Objective-C++ Dialects}.
>> -fnon-call-exceptions  -fdelete-dead-exceptions  -funwind-tables @gol
>> -fasynchronous-unwind-tables @gol
>> -fno-gnu-unique @gol
>> --finhibit-size-directive  -fno-common  -fno-ident @gol
>> +-finhibit-size-directive  -fcommon  -fno-ident @gol
>> -fpcc-struct-return  -fpic  -fPIC  -fpie  -fPIE  -fno-plt @gol
>> -fno-jump-tables @gol
>> -frecord-gcc-switches @gol
>> @@ -14050,35 +14050,27 @@ useful for building programs to run under WINE@.
>> code that is not binary compatible with code generated without that switch.
>> Use it to conform to a non-default application binary interface.
>> -@item -fno-common
>> -@opindex fno-common
>> +@item -fcommon
>> @opindex fcommon
>> +@opindex fno-common
>> @cindex tentative definitions
>> -In C code, this option controls the placement of global variables -defined without an initializer, known as @dfn{tentative definitions} -in the C standard.  Tentative definitions are distinct from declarations +In C code, this option controls the placement of global variables
>> +defined without an initializer, known as @dfn{tentative definitions}
>> +in the C standard.  Tentative definitions are distinct from declarations
>> of a variable with the @code{extern} keyword, which do not allocate storage.
>> -Unix C compilers have traditionally allocated storage for
>> -uninitialized global variables in a common block.  This allows the
>> -linker to resolve all tentative definitions of the same variable
>> +The default is @option{-fno-common}, which specifies that the compiler places
>> +uninitialized global variables in the BSS section of the object file.
> 
> IMO "uninitialized" is confusing because the variables actually
> *are* initialized: with zero.  It's just that the variables don't have
> explicit initializers.  Dito for "uninitialized" in the --help message.
> 
> Johann
> 
> 
>> +This inhibits the merging of tentative definitions by the linker so you get a
>> +multiple-definition error if the same variable is accidentally defined in more
>> +than one compilation unit.
>> +
>> +The @option{-fcommon} places uninitialized global variables in a common block.
>> +This allows the linker to resolve all tentative definitions of the same variable
>> in different compilation units to the same object, or to a non-tentative
>> -definition.  -This is the behavior specified by @option{-fcommon}, and is the default for -GCC on most targets.  -On the other hand, this behavior is not required by ISO
>> -C, and on some targets may carry a speed or code size penalty on
>> -variable references.
>> -
>> -The @option{-fno-common} option specifies that the compiler should instead
>> -place uninitialized global variables in the BSS section of the object file.
>> -This inhibits the merging of tentative definitions by the linker so
>> -you get a multiple-definition error if the same -variable is defined in more than one compilation unit.
>> -Compiling with @option{-fno-common} is useful on targets for which
>> -it provides better performance, or if you wish to verify that the
>> -program will work on other systems that always treat uninitialized
>> -variable definitions this way.
>> +definition.  This behavior does not conform to ISO C, is inconsistent with C++,
>> +and on many targets implies a speed and code size penalty on global variable
>> +references.  It is mainly useful to enable legacy code to link without errors.
>>  @item -fno-ident
>> @opindex fno-ident

Re: [PATCH] PR85678: Change default to -fno-common

[Segher Boessenkool]

On Fri, Oct 25, 2019 at 03:47:10PM +0000, Wilco Dijkstra wrote:
> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
> C feature which is not conforming with the latest C standards.  On many targets
> this means global variable accesses have a codesize and performance penalty.
> This applies to C code only, C++ code is not affected by -fcommon.  It is about
> time to change the default.

Does this actually work on all older OSes (etc.) we support?  Only one
way to find out, of course :-)


Segher

Re: [PATCH] PR85678: Change default to -fno-common

[Georg-Johann Lay]

Wilco Dijkstra schrieb:
> GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
> C feature which is not conforming with the latest C standards.  On many targets
> this means global variable accesses have a codesize and performance penalty.
> This applies to C code only, C++ code is not affected by -fcommon.  It is about
> time to change the default.
> 
> OK for commit?

IIRC using -fno-common might lead to some testsuit fallout because
some optimizations / test cases are sensitive to -f[no-]common.
So I wonder that no adjustments to test cases are needed?

> ChangeLog
> 2019-10-25  Wilco Dijkstra  <wdijkstr@arm.com>
> 
> 	PR85678
> 	* common.opt (fcommon): Change init to 1.
> 
> doc/
> 	* invoke.texi (-fcommon): Update documentation.
> ---
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 0195b0cb85a06dd043fd0412b42dfffddfa2495b..b0840f41a5e480f4428bd62724b0dc3d54c68c0b 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1131,7 +1131,7 @@ Common Report Var(flag_combine_stack_adjustments) Optimization
>  Looks for opportunities to reduce stack adjustments and stack references.
>  
>  fcommon
> -Common Report Var(flag_no_common,0)
> +Common Report Var(flag_no_common,0) Init(1)
>  Put uninitialized globals in the common section.
>  
>  fcompare-debug
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 857d9692729e503657d0d0f44f1f6252ec90d49a..5b4ff66015f5f94a5bd89e4dc3d2d53553cc091e 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -568,7 +568,7 @@ Objective-C and Objective-C++ Dialects}.
>  -fnon-call-exceptions  -fdelete-dead-exceptions  -funwind-tables @gol
>  -fasynchronous-unwind-tables @gol
>  -fno-gnu-unique @gol
> --finhibit-size-directive  -fno-common  -fno-ident @gol
> +-finhibit-size-directive  -fcommon  -fno-ident @gol
>  -fpcc-struct-return  -fpic  -fPIC  -fpie  -fPIE  -fno-plt @gol
>  -fno-jump-tables @gol
>  -frecord-gcc-switches @gol
> @@ -14050,35 +14050,27 @@ useful for building programs to run under WINE@.
>  code that is not binary compatible with code generated without that switch.
>  Use it to conform to a non-default application binary interface.
>  
> -@item -fno-common
> -@opindex fno-common
> +@item -fcommon
>  @opindex fcommon
> +@opindex fno-common
>  @cindex tentative definitions
> -In C code, this option controls the placement of global variables 
> -defined without an initializer, known as @dfn{tentative definitions} 
> -in the C standard.  Tentative definitions are distinct from declarations 
> +In C code, this option controls the placement of global variables
> +defined without an initializer, known as @dfn{tentative definitions}
> +in the C standard.  Tentative definitions are distinct from declarations
>  of a variable with the @code{extern} keyword, which do not allocate storage.
>  
> -Unix C compilers have traditionally allocated storage for
> -uninitialized global variables in a common block.  This allows the
> -linker to resolve all tentative definitions of the same variable
> +The default is @option{-fno-common}, which specifies that the compiler places
> +uninitialized global variables in the BSS section of the object file.

IMO "uninitialized" is confusing because the variables actually
*are* initialized: with zero.  It's just that the variables don't have
explicit initializers.  Dito for "uninitialized" in the --help message.

Johann


> +This inhibits the merging of tentative definitions by the linker so you get a
> +multiple-definition error if the same variable is accidentally defined in more
> +than one compilation unit.
> +
> +The @option{-fcommon} places uninitialized global variables in a common block.
> +This allows the linker to resolve all tentative definitions of the same variable
>  in different compilation units to the same object, or to a non-tentative
> -definition.  
> -This is the behavior specified by @option{-fcommon}, and is the default for 
> -GCC on most targets.  
> -On the other hand, this behavior is not required by ISO
> -C, and on some targets may carry a speed or code size penalty on
> -variable references.
> -
> -The @option{-fno-common} option specifies that the compiler should instead
> -place uninitialized global variables in the BSS section of the object file.
> -This inhibits the merging of tentative definitions by the linker so
> -you get a multiple-definition error if the same 
> -variable is defined in more than one compilation unit.
> -Compiling with @option{-fno-common} is useful on targets for which
> -it provides better performance, or if you wish to verify that the
> -program will work on other systems that always treat uninitialized
> -variable definitions this way.
> +definition.  This behavior does not conform to ISO C, is inconsistent with C++,
> +and on many targets implies a speed and code size penalty on global variable
> +references.  It is mainly useful to enable legacy code to link without errors.
>  
>  @item -fno-ident
>  @opindex fno-ident
> 

[PATCH] PR85678: Change default to -fno-common

[Wilco Dijkstra]

GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
C feature which is not conforming with the latest C standards.  On many targets
this means global variable accesses have a codesize and performance penalty.
This applies to C code only, C++ code is not affected by -fcommon.  It is about
time to change the default.

OK for commit?

ChangeLog
2019-10-25  Wilco Dijkstra  <wdijkstr@arm.com>

	PR85678
	* common.opt (fcommon): Change init to 1.

doc/
	* invoke.texi (-fcommon): Update documentation.
---

diff --git a/gcc/common.opt b/gcc/common.opt
index 0195b0cb85a06dd043fd0412b42dfffddfa2495b..b0840f41a5e480f4428bd62724b0dc3d54c68c0b 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1131,7 +1131,7 @@ Common Report Var(flag_combine_stack_adjustments) Optimization
 Looks for opportunities to reduce stack adjustments and stack references.
 
 fcommon
-Common Report Var(flag_no_common,0)
+Common Report Var(flag_no_common,0) Init(1)
 Put uninitialized globals in the common section.
 
 fcompare-debug
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 857d9692729e503657d0d0f44f1f6252ec90d49a..5b4ff66015f5f94a5bd89e4dc3d2d53553cc091e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -568,7 +568,7 @@ Objective-C and Objective-C++ Dialects}.
 -fnon-call-exceptions  -fdelete-dead-exceptions  -funwind-tables @gol
 -fasynchronous-unwind-tables @gol
 -fno-gnu-unique @gol
--finhibit-size-directive  -fno-common  -fno-ident @gol
+-finhibit-size-directive  -fcommon  -fno-ident @gol
 -fpcc-struct-return  -fpic  -fPIC  -fpie  -fPIE  -fno-plt @gol
 -fno-jump-tables @gol
 -frecord-gcc-switches @gol
@@ -14050,35 +14050,27 @@ useful for building programs to run under WINE@.
 code that is not binary compatible with code generated without that switch.
 Use it to conform to a non-default application binary interface.
 
-@item -fno-common
-@opindex fno-common
+@item -fcommon
 @opindex fcommon
+@opindex fno-common
 @cindex tentative definitions
-In C code, this option controls the placement of global variables 
-defined without an initializer, known as @dfn{tentative definitions} 
-in the C standard.  Tentative definitions are distinct from declarations 
+In C code, this option controls the placement of global variables
+defined without an initializer, known as @dfn{tentative definitions}
+in the C standard.  Tentative definitions are distinct from declarations
 of a variable with the @code{extern} keyword, which do not allocate storage.
 
-Unix C compilers have traditionally allocated storage for
-uninitialized global variables in a common block.  This allows the
-linker to resolve all tentative definitions of the same variable
+The default is @option{-fno-common}, which specifies that the compiler places
+uninitialized global variables in the BSS section of the object file.
+This inhibits the merging of tentative definitions by the linker so you get a
+multiple-definition error if the same variable is accidentally defined in more
+than one compilation unit.
+
+The @option{-fcommon} places uninitialized global variables in a common block.
+This allows the linker to resolve all tentative definitions of the same variable
 in different compilation units to the same object, or to a non-tentative
-definition.  
-This is the behavior specified by @option{-fcommon}, and is the default for 
-GCC on most targets.  
-On the other hand, this behavior is not required by ISO
-C, and on some targets may carry a speed or code size penalty on
-variable references.
-
-The @option{-fno-common} option specifies that the compiler should instead
-place uninitialized global variables in the BSS section of the object file.
-This inhibits the merging of tentative definitions by the linker so
-you get a multiple-definition error if the same 
-variable is defined in more than one compilation unit.
-Compiling with @option{-fno-common} is useful on targets for which
-it provides better performance, or if you wish to verify that the
-program will work on other systems that always treat uninitialized
-variable definitions this way.
+definition.  This behavior does not conform to ISO C, is inconsistent with C++,
+and on many targets implies a speed and code size penalty on global variable
+references.  It is mainly useful to enable legacy code to link without errors.
 
 @item -fno-ident
 @opindex fno-ident