Bug 15486 - [3.3/3.4/4.0 regression] -fdata-sections moves common vars to .bss
Summary: [3.3/3.4/4.0 regression] -fdata-sections moves common vars to .bss
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 3.3.3
: P2 normal
Target Milestone: 4.0.0
Assignee: Eric Botcazou
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2004-05-17 06:47 UTC by Jonathan Larmour
Modified: 2004-12-18 21:56 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 3.3.4 3.4.1 4.0.0
Last reconfirmed: 2004-05-17 11:40:41


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Larmour 2004-05-17 06:47:29 UTC
This is a regression from GCC 3.2.1 (at least).

If I create a file foo.c containing simply:
  int x;
and compile it with:
  arm-elf-gcc -c foo.c
then I can get the following with 'arm-elf-nm foo.o'
00000004 C x

However if I compile with
  arm-elf-gcc -c -fdata-sections foo.c
then with arm-elf-nm I get:
00000000 B x

i.e. x is in the BSS. This results in breaking the long-standing behaviour of
common variables being merged. If you typed the following:
cat >foo2.c
int x=0;
int main(){
  return x;
}
arm-elf-gcc -c -fdata-sections foo.c
arm-elf-gcc -c -fdata-sections foo2.c
arm-elf-gcc -o foo foo.o foo2.o
then you get:
foo2.o(.data.x+0x0): multiple definition of `x'
foo.o(.bss.x+0x0): first defined here

I can see how this behaviour would be beneficial if "-fno-common" was passed to
gcc and if not using "legacy" code (actually the real code I'm having trouble
with is derived from a BSD kernel of about 4 years ago, so not that old), but in
the absence of -fno-common, -fdata-sections should not operate on uninitialised
data. Perhaps the default should be -fno-common, and I should be required to
pass -fcommon to get the old behaviour, I'm not bothered. Or alternatively
perhaps this treatment of uninitialised data should only happen with a separate
-fbss-sections option (for symmetry with -ffunction-sections and -fdata-sections).
Comment 1 Andrew Pinski 2004-05-17 11:27:19 UTC
You can workaround it by -fno-zero-initialized-in-bss.
Comment 2 Jonathan Larmour 2004-05-17 11:32:22 UTC
I think in my case not using -fdata-sections would be a better (or less bad :-))
workaround than increasing the initialized data section that much. But might be
useful for others to know if they search for this bug in the archives.
Comment 3 Eric Botcazou 2004-05-17 11:38:16 UTC
The workaround doesn't work for the testcase since 'x' is not initialized at all.
Comment 4 Eric Botcazou 2004-05-17 11:40:40 UTC
Regression from GCC 3.2.x.  Not sure if this is "wrong-code" though.
Comment 5 Jonathan Larmour 2004-05-17 21:51:02 UTC
No I wouldn't consider it wrong code. I think it's sensible to allow nice modern
programs to benefit from garbage collection of BSS sections. But since it breaks
older code, only as an option (whether it be default on or default off I'm not
bothered).
Comment 6 Giovanni Bajo 2004-06-06 03:58:36 UTC
Retargeting to 3.4.1, being a regression on that release branch.
Comment 7 Mark Mitchell 2004-06-19 17:46:40 UTC
Postponed until GCC 3.4.2.
Comment 8 Richard Earnshaw 2004-06-22 10:26:38 UTC
I'm not convinced this is even a bug.

Looking at the assembly code generated for the test case
int x;
we see:

        .file   "foo.c"
        .global x
        .section        .bss.x,"aw",%nobits
        .align  2
        .type   x, %object
        .size   x, 4
x:
        .space  4

So indeed the compiler has done as asked and given X a section of its own
(.bss.x).  I'm not aware that it is possible to do this using the common model -
at least, not in ELF.

Section 6.2.2 of the C99 rationale describes 4 linkage models: Common, Relaxed
Ref/Def, Strict Ref/Def and Initialization.  GCC normally implements the Relaxed
Ref/Def model, since this is tradionally what Unix uses; but I don't think it
would be unreasonable to assert that -fdata-sections requires the Strict Ref/Def
model.

My inclination would be to document that -fdata-sections implies -fno-common.
Comment 9 Jonathan Larmour 2004-06-22 11:12:02 UTC
Firstly the compiler has arguably not done as asked, because -fdata-sections did
not used to affect the bss section at all. That makes this a regression. As I
suggested, perhaps this should be a separate -fbss-sections option.

Secondly, what you are suggesting is deliberately breaking traditional unix
code, including moderately recent BSD kernels, as that's where the eCos code
came from (specifically TCP/IP). Sometimes such a thing could be justified if an
optimisation cannot be achieved any other way, but that is not true here: a
separate -fbss-sections option or similar (-fdata-sections -fno-bss-sections if
you want to retain this behaviour for -fdata-sections) could do this.

Finally, this is not quite the same as saying -fdata-sections implies
-fno-common. -fno-common is used to, essentially, put BSS data in the .data
section. That is not what -fdata-sections is now doing, although it is what one
would expect it to do if you did pass -fno-common (if you see the distinction I
mean). So it would be incorrect to document that -fdata-sections implies
-fno-common.
Comment 10 Richard Earnshaw 2004-06-22 11:33:29 UTC
Just because the compiler changed behaviour doesn't necessarily make it a
regression.  Perhaps the change was intentional.

It could be equally argued that failing to put uninitialized data in separate
sections was the bug that was being fixed when the change was introduced.  It
would be necessary to do some archeology to find out which patch caused the
change, and why it was proposed in the first place.
Comment 11 Jonathan Larmour 2004-06-22 19:37:14 UTC
I'm not arguing against the current behaviour as such. I think the current
behaviour would be a good idea as it allows GC of BSS sections; but it should
not be rolled into an existing option with an existing known action, breaking
long-standing behaviour used in lots of code. Or if it is, there should be a way
to stop it in this case.

Anyway, after some archaeology (:-)), I have found the patch that did it:
http://gcc.gnu.org/ml/gcc-patches/2002-03/msg00921.html
Specifically moving resolve_unique_section() before the check of
DECL_SECTION_NAME which is:

  /* If the decl has been given an explicit section name, then it
     isn't common, and shouldn't be handled as such.  */
  if (DECL_SECTION_NAME (decl) || dont_output_data)
    ;
  /* We don't implement common thread-local data at present.  */
  else
    [etc.]
It isn't clear what the rationale for this change was: the "more fallout"
referred to here is I believe subsequent to
http://gcc.gnu.org/ml/gcc-patches/2002-03/msg00768.html perhaps which is in turn
after Kaveh's changes to add -fno-zero-initialized-in-bss in
http://gcc.gnu.org/ml/gcc-patches/2002-03/msg00062.html

Jason presumably had come across some reason why Kaveh's change broke something.
But I don't think it was deliberately intended to remove common variable support
when using -fdata-sections. Given the other purpose of resolve_unique_section()
I would guess it's to do with the correct implementation of transparent unions,
which are used in several high-profile places in glibc, including most socket
functions and an argument to the wait() family of functions. Are there other
ways DECL_SECTION_NAME could be set before this point?

As to how to address this then, I'm not too hot on GCC internals, but the old
behaviour could be returned simply by making the above cited if actually:

  if (DECL_ONE_ONLY(decl) && DECL_SECTION_NAME (decl) || dont_output_data)

This would then allow the else clauses to be followed as expected. This is on
the assumption that the reasoning behind Jason's change is as per above. If it's
something else, hopefully the above info helps someone more knowledgable than I
work out what the rationale is.

Although I would say that someone should consider allowing -fdata-sections (or a
new -fbss-sections option) to operate on the BSS as well in future, due to the
space savings that could give, as long as there's a way to get back the current
behaviour.
Comment 12 Jonathan Larmour 2004-06-22 22:13:09 UTC
That should of course be
 if (DECL_SECTION_NAME (decl) || dont_output_data && DECL_ONE_ONLY(decl))

E&OE :-)
Comment 13 Andrew Pinski 2004-08-17 23:17:56 UTC
Not really a bug as the C standards say this case is invalid so closing.
Comment 14 Jonathan Larmour 2004-08-18 01:39:07 UTC
Richard Earnshaw gave a most helpful analysis describing the various linkage
models mentioned in 6.22 of the C99 standard rationale. I quote from there about
the relaxed ref/def model that GCC previously implemented: "The UNIX operating
system C compiler and linker implement this model, which is recognized as a
common extension to the C language (see §K.5.11). UNIX C programs which take
advantage of this model are standard conforming in their environment, but are
not maximally portable (not strictly conforming)."

In other words, the programs are indeed standard conforming, not "invalid".
Strict conformance is akin to -pedantic - preventing an extension.

Are you perhaps saying that 6.9 para 5 of C99 mandates only one definition?
Actually that does not follow, as that only applies if the object is declared
with an external definition. It does not apply, as in the "legacy" code in
question, if the object is always defined (therefore becomes common), and never
declared with extern. The standard does not mandate breaking such code.

The code I have isn't really legacy code - it's only a few years old and was
taken from BSD post-1999 even. At the very least to be completely pedantic about
standards conformance, and break compilation of such code which has been
deliberately been maintained to work to date goes against the GNU spirit of
making things work. Especially when the change is clearly unintentional. I've
even suggested what I believe to be a simple fix.

Breaking a large body of past code should not be something done lightly,
certainly not in a patch that doesn't mention it and was intended to fix other
issues; and it isn't a decision that should be taken by one person just in this
bug. At the very least there are other ramifications:

I suggest if Andrew wants this change to be permanent, he needs to submit a
change to the -fno-common, and __attribute__((nocommon)),
__attribute__((section(...))) documentation which are all clearly wrong if
common data is eliminated. There are also quite a few comments in GCC that need
to be fixed to reflect this proposed new reality; and some code that could
probably now be removed.

I've reopened this bug to indicate that either this issue is still live as gcc
should not be suddenly (and without warning or prior deprecation) breaking
common Unix code; or that someone needs to fix the documentation and other bits
as above. At least that way with a proposed doc patch, the removal of common
variable support can be brought to a wider audience than this bug report.
Comment 15 Andrew Pinski 2004-08-18 02:22:10 UTC
What is invalid about this problem is that x is initialized in one TU but not another, I will note note that 
on darwin this will fail no matter what.
Comment 16 Jonathan Larmour 2004-08-18 04:42:24 UTC
Then remove the initialiser. The problem is just the same. I didn't realise it
would make a difference to anyone's opinion.

In my previous reply, I did lose a little perspective rather mistakenly (the
passage of time since this bug was submitted I guess) - this misbehaviour is
specific to -fdata-sections. What this means is that gcc behaves differently
with -fdata-sections and without, and breaks code with -fdata-sections, and not
without, which is not meant to happen.

Again: the change was not intentional, and is almost certainly easily restored
to the previous behaviour.
Comment 17 Mark Mitchell 2004-08-19 21:18:30 UTC
Jason, would you please comment on this discussion, given that one of your
patches seems to be at issue?
Comment 18 Jason Merrill 2004-08-20 21:09:11 UTC
Subject: Re:  [3.3/3.4/3.5] -fdata-sections moves
 common vars to .bss

I believe that the problem which my patch was addressing was that Kaveh's
patch was causing variables to end up in .bss which needed to be in a
special section.  I'm not sure why that would be, since
asm_emit_uninitialized does call resolve_unique_section.  If you want to
try reverting my patch and seeing if anything breaks, I'm open to that.

Jason
Comment 19 Mark Mitchell 2004-08-29 19:09:47 UTC
Postponed until GCC 3.4.3.
Comment 20 Mark Mitchell 2004-10-31 02:07:23 UTC
Postponed until GCC 3.4.4.
Comment 21 Eric Botcazou 2004-12-09 13:15:56 UTC
> I believe that the problem which my patch was addressing was that Kaveh's
> patch was causing variables to end up in .bss which needed to be in a
> special section.  I'm not sure why that would be, since
> asm_emit_uninitialized does call resolve_unique_section.  If you want to
> try reverting my patch and seeing if anything breaks, I'm open to that.

I don't think your patch can be reverted: the point is that
resolve_unique_section must be called before DECL_SECTION_NAME is tested.

I'm going to try to restore the original behaviour with an ad-hoc approach.
Comment 22 Eric Botcazou 2004-12-14 08:21:41 UTC
Patch at http://gcc.gnu.org/ml/gcc-patches/2004-12/msg00909.html
Comment 23 CVS Commits 2004-12-18 21:50:05 UTC
Subject: Bug 15486

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	ebotcazou@gcc.gnu.org	2004-12-18 21:49:56

Modified files:
	gcc            : ChangeLog varasm.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: fdata-sections-1.c 

Log message:
	PR middle-end/15486
	* varasm.c (asm_emit_uninitialised): Return early if
	a custom section is requested.
	(assemble_variable): Revert 2002-03-15 patch.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.6883&r2=2.6884
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/varasm.c.diff?cvsroot=gcc&r1=1.468&r2=1.469
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.4781&r2=1.4782
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/fdata-sections-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 24 Eric Botcazou 2004-12-18 21:56:35 UTC
See http://gcc.gnu.org/ml/gcc-patches/2004-12/msg01295.html
Fixed in the upcoming 4.0.0 only, as per the RM's ruling.