Fix multiple definition errors

[runlevel5]

What

This PR addresses failed compilation with GCC 10

which was reported in #261:

$ make
compiling semian.c
compiling tickets.c
linking shared-object semian/semian.so
/usr/bin/ld: semian.o:(.bss+0x60): multiple definition of `eSyscall'; resource.o:(.bss+0x28): first defined here
/usr/bin/ld: semian.o:(.bss+0x58): multiple definition of `eTimeout'; resource.o:(.bss+0x20): first defined here
/usr/bin/ld: semian.o:(.bss+0x50): multiple definition of `eInternal'; resource.o:(.bss+0x18): first defined here
/usr/bin/ld: semian.o:(.bss+0x48): multiple definition of `id_wait_time'; resource.o:(.bss+0x10): first defined here
/usr/bin/ld: semian.o:(.bss+0x40): multiple definition of `id_timeout'; resource.o:(.bss+0x8): first defined here
/usr/bin/ld: semian.o:(.bss+0x38): multiple definition of `system_max_semaphore_count'; resource.o:(.bss+0x0): first defined here
/usr/bin/ld: sysv_semaphores.o:(.bss+0x10): multiple definition of `eSyscall'; resource.o:(.bss+0x28): first defined here
/usr/bin/ld: sysv_semaphores.o:(.bss+0x0): multiple definition of `eInternal'; resource.o:(.bss+0x18): first defined here
/usr/bin/ld: sysv_semaphores.o:(.bss+0x8): multiple definition of `eTimeout'; resource.o:(.bss+0x20): first defined here
/usr/bin/ld: tickets.o:(.bss+0x10): multiple definition of `eSyscall'; resource.o:(.bss+0x28): first defined here
/usr/bin/ld: tickets.o:(.bss+0x8): multiple definition of `eTimeout'; resource.o:(.bss+0x20): first defined here
/usr/bin/ld: tickets.o:(.bss+0x0): multiple definition of `eInternal'; resource.o:(.bss+0x18): first defined here
collect2: error: ld returned 1 exit status
make: *** [Makefile:261: semian.so] Error 1

GCC 10 now default -fno-common. Many linker errors are now reported.

[sirupsen]

Have you encountered the cause for the multiple definitions here (been a while since I've looked at the C-ext)? Why is this the best fix?

[dalehamel]

Thank you for your pull request, but I don't think I agree with this solution.

My speculation is that some behavior has changed in GCC 10. Are you able to repro this with GCC 8 or 9? Clang?

The source code should only result in a single definition for these values (I didn't spot check them all), but enough to be pretty confident about this). If some compiler behavior results in these symbols being defined multiple times, I don't think that simply ignoring this / allowing it is a good workaround.

If there is some other error in our make files that allow this, we should fix that. Allowing multiple definitions seems like it could lead to unpredictable / undefined behaviro.

[runlevel5]

@dalehamel Thanks for information. I do speculate that is behaviour change of GCC 10. Let me go back to you after I verify this hypothesis.

[runlevel5]

@dalehamel I can confirm that GCC9 does not have this same issue. I think the issue is related GCC10

Ref: https://gcc.gnu.org/gcc-10/porting_to.html
## Default to -fno-common
A common mistake in C is omitting extern 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 -fno-common, which means a linker error will now be reported. To fix this, use extern in header files when declaring global variables, and ensure each global is defined in exactly one C file. If tentative definitions of particular variables need to be placed in a common block, __attribute__((__common__)) can be used to force that behavior even in code compiled without -fcommon. As a workaround, legacy C code where all tentative definitions should be placed into a common block can be compiled with -fcommon.


      int x;  // tentative definition - avoid in header files

      extern int y;  // correct declaration in a header file

[dalehamel]

A common mistake in C is omitting extern 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.

This is news to me, it is such a common practice that it's a little funny to call it a mistake... I guess because they are for variables an not function declarations, I can see why.

In your PR i see you're going with the attribute approach, I am curious how this will work with Clang. As an aside, we should probably try building semian with clang just to enforce good portability practices. Why did you choose this approach over just extern?

[runlevel5]

@dalehamel

In your PR i see you're going with the attribute approach, I am curious how this will work with Clang

Good one. Let me come back to you on that.

Why did you choose this approach over just extern?

I am not feeling 100% confident with semian codebase at this stage. Therefore I am leaning toward solution that is the least disruptive.

[runlevel5]

@damianthe I confirm my PR also work on clang9 and clang10.

[runlevel5]

@damianthe I've amended the PR with extern approach

[runlevel5]

I am going to close this PR instead. Once GCC10 has become default on all mainstream Linux distros, I think we could look into this issue again.

[runlevel5]

Re-open this PR because GCC 10.1 is officially released.

[runlevel5]

I believe the core team would address the issue so I am closing this PR. Thanks all for helping with PR review.