[master] Fails to compile with compile flag -fno-common (that is default for GCC >=10)

[hartwork]

Hi!

GCC 10 enables -fno-common by default and the code of spacenavd does not compile without error with that flag enabled (full log). Are you open to a pull request along the lines of this proposed patch? The original downstream bug report is at https://bugs.gentoo.org/706966 .

Thanks and best, Sebastian

Related: https://github.com/FreeSpacenav/spacenavd/blob/8acf883d97ea30a098621ac7128e5d40ba90f496/src/spnavd.h#L52-L53

[jtsiomb]

Interesting. There are two ways to solve this issue. One is to throw an extern in front of the declaration in the header file, and then duplicate the declaration without the extern keyword in one source file, like what your proposed change does (there are also some unrelated naming changes in there?). Another, is to explicitly pass the previously implicit -fcommon flag to the compiler.

I'm curious why the GCC team thinks that making -fno-common the default makes any sense, and I lean towards the second solution for two reasons.

The first and most important is a practical one: duplicating declarations in multiple files is redundant and pointless. If the type needs to change at some point, it has to change in two places instead of one, and by looking at the header file, it's not necessarilly obvious in which source file the non-extern declaration is.

The second reason is that by my understanding of C, this is just wrong. The C99 standard, which I happen to have conveniently in a PDF here (although I strictly use C89 in this project), states in section 6.2.2 "Linkage of identifiers" paragraph 2: "In the set of translation units and libraries that constitutes an entire program, each declaration of a particular identifier with external linkage denotes the same object...". And just to make sure this applies here in 6.9.2 "External object definitions" in example 1, an identifier declared in file-scope with int i4; is specified to be a "tentative definition, external linkage".

Unless I'm reading the standard wrong, this change contradicts the C standard, and certainly flies in the face of existing practice, and will introduce a lot of compatibility problems with existing code. The reasoning given in the documentation for abandoning the default of "-fcommon" that "This behavior is inconsistent with C++" is laughable, and then the assertion that "and on many targets implies a speed and code size penalty on global variable references" is vague and certainly not the case in any platform I know of.

[jtsiomb]

Closing the issue, assuming change 081182f fixes the problem. If not, please re-open.

[hartwork]

Hi John, thanks for that commit.

I have no stakes regarding their change of defaults.

Judging from my local GCC 9.2.0 manpage their understanding is different:

  -fno-common
      In C code, this option controls the placement of global variables defined
      without an initializer, known as tentative definitions in the C standard.
      Tentative definitions are distinct from declarations of a variable with
      the "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 in different compilation
      units to the same object, or to a non-tentative definition.  This is the
      behavior specified by -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 -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 -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.

[hartwork]

[..] and will introduce a lot of compatibility problems with existing code.

Indeed, you might find https://bugs.gentoo.org/705764 of interest in that regard.

[hartwork]

~I'm afraid we need to re-open this issue — compilation (or linking) fails despite the recent addition of -fcommon (as can be seen in the log):~

• ~build log~
• ~bug report~

~Unless dev.c:(.text+0x33e): undefined reference to 'drop_xinput' (#19) is the actual cause. Needs more investigation.~

[hartwork]

I have a local copy of GCC 10.0.1 now and can confirm that 081182f1675bb69eec58d92698f1ba2f23466d3f indeed fixed the build.

[trofi]

CFLAGS=-fno-common ./configure && make -j9 still breaks the build on gcc-10. Mostly due to user/default ordering (https://bugs.gentoo.org/708648#c11):

$ gcc -pedantic -Wall -g -O2 -fno-strict-aliasing -fcommon -I./src -I/usr/local/include -fno-common -c src/cfgfile.c -o src/cfgfile.o

Any reason not to make single definition source code change instead of using compiler-specific -fcommon option? Specifically definition-site evpool_size is clearer when explicitly expressed with extern:

The fix would be:

diff --git a/src/serial/sball.c b/src/serial/sball.c
index cfbbdd1..0a156b5 100644
--- a/src/serial/sball.c
+++ b/src/serial/sball.c
@@ -63 +63 @@ static struct event *ev_free_list;
-int evpool_size;
+extern int evpool_size;
diff --git a/src/spnavd.c b/src/spnavd.c
index c51b120..5755d10 100644
--- a/src/spnavd.c
+++ b/src/spnavd.c
@@ -39,0 +40,4 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
+/* globals */
+struct cfg cfg;
+int verbose;
+
diff --git a/src/spnavd.h b/src/spnavd.h
index fa0a916..3713702 100644
--- a/src/spnavd.h
+++ b/src/spnavd.h
@@ -52,2 +52,2 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
-struct cfg cfg;
-int verbose;
+extern struct cfg cfg;
+extern int verbose;

[trofi]

The rationale behind the gcc change is in https://gcc.gnu.org/PR85678

[jtsiomb]

CFLAGS=-fno-common ./configure && make -j9 still breaks the build on gcc-10. Mostly due to user/default ordering (https://bugs.gentoo.org/708648#c11):

Of course if you explicitly specify -fno-common it will break. So stop doing that, and it will not break.

Any reason not to make single definition source code change instead of using compiler-specific -fcommon option?

I discussed this extensively in this thread. I don't know how you managed to miss it. Scroll up.

[trofi]

CFLAGS=-fno-common ./configure && make -j9 still breaks the build on gcc-10. Mostly due to user/default ordering (https://bugs.gentoo.org/708648#c11):

Of course if you explicitly specify -fno-common it will break. So stop doing that, and it will not break.

I interpret your response as -fno-common configuration is not supported and will not be supported by this project.

Gentoo (and other downstreams and individuals) will have to add a workaround for users with system-wide CFLAGS=-fno-common setting.

Any reason not to make single definition source code change instead of using compiler-specific -fcommon option?

I discussed this extensively in this thread. I don't know how you managed to miss it. Scroll up.

I did read it as points to defend existing code being correct. I disagree with them and find some of them contradictory. But I don't want to argue them. It's all a matter of preference.

I hoped that new (not really new) failure mode would bring a new point to consider the change. I interpret your response as new failure mode did have no effect on decision and -fcommon will stay (and will stay overridable by user's CFLAGS).

To clarify: I'm here not to convince you otherwise of force my point of view on you. It's your project. I just report downstream build failure and exploring a fix that would last and be acceptable upstream.

On a related note I still don't understand if evpool_size sharing across modules is accidental or intentional. -fcommon implies it's intentional while the code seem to treat it as local for both smag_event.c and sball.c. Sounds like a bug and I'm glad to see new gcc makes such suspicious code obvious. You might want to add a source code comment if it's intentional.

In Gentoo's experience most projects that fail to build with -fno-common have accidental sharing (iozone, xmms2, binutils, many others), have accidental redundant definitions where declarations only are expected (openrc, freeglut, many others).

Thanks!

[jtsiomb]

I interpret your response as -fno-common configuration is not supported and will not be supported by this project. Gentoo (and other downstreams and individuals) will have to add a workaround for users with system-wide CFLAGS=-fno-common setting.

Correct, unless someone comes up with a tangible substantial benfit of compiling with -fno-common.

On a related note I still don't understand if evpool_size sharing across modules is accidental or intentional.

I'll look into that. Of course if it's not intentional, then the correct fix is to make it static, which has no bearing on the issue of -fcommon vs -fno-common.

Edit:

have accidental redundant definitions where declarations only are expected (openrc, freeglut, many others).

I'm one of the maintainers of freeglut and I can't remember a bug report about this gcc flag. If you reported something and it went unnoticed/unanswered, send me the link to the bug report via email and I'll look into it.

[jtsiomb]

Sharing of evpool_size was indeed accidental, fixed in commit 7c271fa

[trofi]

have accidental redundant definitions where declarations only are expected (openrc, freeglut, many others).

I'm one of the maintainers of freeglut and I can't remember a bug report about this gcc flag. If you reported something and it went unnoticed/unanswered, send me the link to the bug report via email and I'll look into it.

https://github.com/dcnieho/FreeGLUT/pull/76

[jtsiomb]

Yeah, that's why I never saw that. That's not the correct place for bug reports for freeglut. The project bug tracker is on sourceforge, or you can post in the freeglut-developer mailing list, if you want anyone to see it. I'll take a look.

[michaelforney]

Correct, unless someone comes up with a tangible substantial benfit of compiling with -fno-common.

Relying on -fcommon to merge these duplicate definitions is invalid in C99 as well as C89.

The second reason is that by my understanding of C, this is just wrong. The C99 standard, which I happen to have conveniently in a PDF here (although I strictly use C89 in this project), states in section 6.2.2 "Linkage of identifiers" paragraph 2: "In the set of translation units and libraries that constitutes an entire program, each declaration of a particular identifier with external linkage denotes the same object...". And just to make sure this applies here in 6.9.2 "External object definitions" in example 1, an identifier declared in file-scope with int i4; is specified to be a "tentative definition, external linkage".

Unless I'm reading the standard wrong, this change contradicts the C standard, and certainly flies in the face of existing practice, and will introduce a lot of compatibility problems with existing code. The reasoning given in the documentation for abandoning the default of "-fcommon" that "This behavior is inconsistent with C++" is laughable, and then the assertion that "and on many targets implies a speed and code size penalty on global variable references" is vague and certainly not the case in any platform I know of.

It does not contradict the C standard. As you mentioned, each translation unit that includes spnavd.h results in a tentative definition for these identifiers with external linkage.

Since there is no external definition for these identifiers, there is an implicit external definition at the end of each of these translation units:

C99 6.9.2p2

A declaration of an identifier for an object that has file scope without an initializer, and without a storage-class specifier or with the storage-class specifier static, constitutes a tentative definition. If a translation unit contains one or more tentative definitions for an identifier, and the translation unit contains no external definition for that identifier, then the behavior is exactly as if the translation unit contains a file scope declaration of that identifier, with the composite type as of the end of the translation unit, with an initializer equal to 0.

However, an identifier with external linkage used somewhere in the program must have exactly one external definition:

C99 6.9p5

An external definition is an external declaration that is also a definition of a function (other than an inline definition) or an object. If an identifier declared with external linkage is used in an expression (other than as part of the operand of a sizeof operator whose result is an integer constant), somewhere in the entire program there shall be exactly one external definition for the identifier; otherwise, there shall be no more than one.