Link error for identifier FirstTime

[bzinberg]

In CI here, clang complained of a double definition.

This looks like a valid complaint to me, though as we saw here, gcc successfully compiles the same code. Presumably this means the code is incorrect (i.e. compiler behavior is unspecified) but gcc is being lenient in some way.

Quoting @giordano:

maybe the issue is that the global variable in the header file is being defined twice in two different object files. It's just a completely bad idea to define variables in header files (without being extern at least).

I've personally never seen an issue like this before, for (I think) two reasons: (1) I've never seen a non-static, non-extern global variable in a header file; (2) all the codebases I've worked in have had include guards on the header files, so re-declaration was not happening.

What we need to do

We should fix the link error in a way that does not change program behavior.

As far as I can see, the only sane way this program would compile is if the offending line is being treated as an extern declaration. However I don't have iron-clad proof of this, and I think it's a statement about the behavior of GCC that is not mandated by the C standard.

Of course, we could also start reasoning about the program itself and what it's supposed to do, and/or contact the author 🙂

Attempting to get a full explanation

According to Wikipedia:

If neither the extern keyword nor an initialization value are present, the statement can be either a declaration or a definition. It is up to the compiler to analyse the modules of the program and decide.

According to cppreference

For objects, a declaration that allocates storage (automatic or static, but not extern) is a definition, while a declaration that does not allocate storage (external declaration) is not.

extern int n; // declaration
int n = 10; // definition

Sadly, the above quote does not explicitly say whether the statement

int n;

with no storage-class specifier and no initialization must be considered a definition, or must be considered only a declaration, or whether it's up to the toolchain. (Wikipedia apparently says it's up to the toolchain.)

As far as I can tell, this line is getting inlined twice by the preprocessor (since there is no include guard), and the clang error above seems to indicate that clang is considering at least two of the copies of that line to be definitions (since the only other use of that name in the codebase is definitely neither a definition nor a declaration). Perhaps gcc chooses not to consider more than one of those copies a definition.

On the other hand, if all copies of that line are considered to be declarations and not definitions, then the following excerpt from cppreference:

If no storage-class specifier is provided, the defaults are:

• extern for all functions
• extern for objects at file scope
• auto for objects at block scope

implies that the line is equivalent to

extern long FirstTime;

[bzinberg]

All that said, given that the name is only referred to in lrsnashlib.c, it seems like we might as well declare FirstTime to be static? AFAICS, for this specific program, extern and static would behave the same way, unless the user is also running some other program that links against lrsnashlib and tries to mess with the value of FirstTime (which would only be allowed in the extern case, not the static case).

[bzinberg]

Never mind, LRSLib.jl does want to modify the value of FirstTime. extern it is.

[blegat]

Thanks for investigating this and detailing your reasoning so precisely here. Another fix could be to add add an include guard. Otherwise, yes indeed I think if the variable is in the .h, it's because the user is allowed to mess with it.

[giordano]

Another fix could be to add add an include guard.

The error happens at link-time because the symbol is defined in two different object files, an include guard would be useless

[00:48:40] duplicate symbol '_FirstTime' in:
[00:48:40]     /tmp/lrsnash-b87646.o
[00:48:40]     /tmp/lrsnashlib-d1516b.o
[00:48:40] ld: 1 duplicate symbol for architecture x86_64
[00:48:40] clang-11: error: linker command failed with exit code 1 (use -v to see invocation)
[00:48:40] make: *** [makefile:227: lrsnash-shared] Error 1

[bzinberg]

@giordano, AFAICS, the declaration/definition in lrsnash-b87646.o comes from inlining lrsnashlib.h, so adding an include guard would result in only one declaration/definition.

[oyamad]

One option is to update to the latest v7.1 (download) (if it is not too much work).

The declaration is removed from lrsnashlib.h, and it is now defined as static long FirstTime in lrsnashlib.c.

(EDIT: Maybe we need a definition in lrsnashlib.h, as mentioned in #2 (comment) and #2 (comment)?)

[bzinberg]

@oyamad yes, I think we need either to keep the variable extern or expose a reset() function. Fine with either one, though the latter would require a coordinated change in LRSLib.jl.

[bzinberg]

All for updating to latest upstream lrslib too, but yeah it might be more effort than we have available right now.