#77 Conditionally add -fcommon to CFLAGS
Merged a month ago by churchyard. Opened a month ago by law.
Unknown source common-handling  into  master

file modified
+12

@@ -148,6 +148,18 @@

  you can pass `-z undefs` to ld (written as `-Wl,-z,undefs` on the gcc

  command line).  The latter needs binutils 2.29.1-12.fc28 or later.

  

+ ### Legacy -fcommon

+ 

+ Since version 10, [gcc defaults to `-fno-common`](https://gcc.gnu.org/gcc-10/porting_to.html#common).

+ Builds may fail with `multiple definition of ...` errors.

+ 

+ As a short term workaround for such failure,

+ it is possible to add `-fcommon` to the flags by defining `%_legacy_common_support`.

+ 

+     %define _legacy_common_support 1

+ 

+ Properly fixing the failure is always preferred!

+ 

  # Individual compiler flags

  

  Compiler flags end up in the environment variables `CFLAGS`,

file modified
+1 -1

@@ -230,7 +230,7 @@

  %_ld_as_needed		1

  %_ld_as_needed_flags	%{?_ld_as_needed:-Wl,--as-needed}

  

- %__global_compiler_flags	-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches %{_hardened_cflags} %{_annotated_cflags}

+ %__global_compiler_flags	-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches %{_hardened_cflags} %{_annotated_cflags}%{?_legacy_common_support: -fcommon}

  

  # Automatically trim changelog entries after 2 years

  %_changelog_trimtime	%{lua:print(os.time() - 2 * 365 * 86400)}

file modified
+4 -1

@@ -6,7 +6,7 @@

  

  Summary: Red Hat specific rpm configuration files

  Name: redhat-rpm-config

- Version: 148

+ Version: 149

  Release: 1%{?dist}

  # No version specified.

  License: GPL+

@@ -207,6 +207,9 @@

  %{_rpmconfigdir}/macros.d/macros.kmp

  

  %changelog

+ * Thu Jan 23 2020 Jeff Law <law@redhat.com> - 149-1

+ - Allow conditionally adding -fcommon to CFLAGS by defining %%_legacy_common_support

+ 

  * Mon Jan 20 2020 Florian Weimer <fweimer@redhat.com> - 148-1

  - Reenable annobin after GCC 10 integration (#1792892)

  

gcc-10 is expected to land in Fedora 32 any day now. gcc-10 changes the C compiler so that it no longer supports "common" symbols by default. This can cause packages which previously built fine to get errors at link time for multiply defined symbols.

We have identified approximately 460 packages that rely on "common" symbols for C that are going to fail to build from source when gcc-10 lands. Ideally those packages will be fixed by their owners, but the process can be failure prone, particularly if the package has common symbols in shared libraries. Therefore we are proposing an opt-out mechanism so that packages that rely on "common" symbol handling can trivially restore the old compiler behavior. Packages that honor flags injection and need the opt-out can add "%define _legacy_common_support 1" to their .spec file to restore the old behavior.

Note this change only affects C and thus the change only affects %build_cflags. Changing %{optflags} would be highly undesirable as such changes would affect other languages.

We expect that of the ~460 packages that require legacy "common" symbol handling that approximately 150 do not honor flags injection. Typically those packages provide their own CFLAGS via something like this "CFLAGS=%{optflags} ..." Those packages will need to be fixed individually by adding "-fcommon" to their CFLAGS.

Note this change is independent of the LTO work and can proceed into Fedora without adverse impacts immediately.

Note that in another PR (LTO related) someone requested that I use shell conditionals. Those don't work particularly cleanly here because we're not running shell commands. It'd have to be something like

[ "%_legacy_common_support" = 1 ] && echo -fcommon || true

If you'd prefer a shell conditional, I'll make that change. Just let me know. I've tested both styles.

I think those packages should be fixed instead of working around it here.

That's fine. But I can't produce a proper fix for all 460+ packages and that sounds like an awful lot of packages to have failing when gcc-10 drops. I'll pass along the list of packages and y'all can pester the package owners.

In contrast, if the opt-out is in place, I am willing to add the marker to the affected packages and contact the owners to install a proper fix (and remove the marker once complete). Also note that an opt-out is the direction openSUSE and Ubuntu are going on this issue as well.

Ultimately it's your call. I just want the introduction of gcc-10 to be minimally invasive as possible. I'll move on to other issues that need to be addressed if y'all don't want the opt-out.

Just open bugs for all those components and maintainers will have to fix them. Or wait until mass rebuild and FTBFS bugs will be opened.

My offer to help is with the opt-out and answering questions in this space. Opening 460 FTBFS bugs, no thank you. I'll just drop the PR.

Pull-Request has been closed by law

a month ago

This seems quite reasonable to me. Use of such a central opt-out is also much easier to track than per-package hacks to fix.

As for shell vs macro conditionals: no need for shell in this case. This isn't centrally enabled, so packages will never need to tell rpm to disable a thing, which is the problematic case with macro conditionals.

I thought so as well. I didn't want to make the background any longer, but the opt-out was also meant to break the problem down into something more manageable and reduce the time crunch on the package maintainers.

ie, we get the opt-out installed on the packages that need it. They're now marked as needing upstream TLC, but are at least working. We could then shift focus to the ~150 packages that do not honor flags injection and are thus going to require a higher degree of human intervention to workaround/fix.

But it sounds like Igor strongly doesn't want to move forward on the opt-out mechanism. I think that's puts an unnecessary time-crunch burden on the package maintainers, but it's not my decision to make.

Can we please have this?

Pull-Request has been reopened by churchyard

a month ago

Yeah, I appreciate the sentiment Igor, but I am fairly annoyed that I can't build a kernel right now and there are multiple places that need to be fixed. While I am sure upstream will be fixed shortly, this is a package that is rebuilt daily. Some of these fixes (in kernel and elsewhere) aren't just copy/paste trivial.

Let's have this as a short term workaround and possibly drop it in couple months. I can add changelog and release. Will merge on Friday if there is no public outcry.

rebased onto a1d75b7

a month ago

Bumped version, added a changelog entry, removed extra unneeded space between %{optflags} and %{?_legacy_common_support: -fcommon}

rebased onto 069cee0

a month ago

Added documentation. @jforbes (or other interested maintainers) could you please review it?

The added documentation looks fine, it is pretty straightforward.

Thanks. As promised, I plan to merge tmrw.

Sorry, you need to update __global_compiler_flags because build_flags does not affect optflags, which is still used by many packages.

rebased onto 3e759e7

a month ago

Limiting it to build_cflags was intentional. It avoids having the flag leak out into flags for other languages if someone had a mixed language package and needed -fcommon for the C bits.

It certainly limits the usefulness as, yes, many packages use optflags rather than build_cflags. But I wanted to take the safest conservative path. I don't think it'll cause observable problems if (for example) C++ code was built with -fcommon, but I haven't studied the issue in any depth.

So I'm slightly less comfortable with it showing up in optflags, but not enough that I would object to that path. The increase in coverage is probably dramatically higher than any fallout would be.

Right, I had not considered that. Unfortunately, I think there is a correlation between packages needing -fcommon and those using %optflags (both tend to be old code bases), so I agree with you that applying the explicit opt-out to all language flags is likely the way to go.

I think that the affected packages who need -fcommon for C code, but -fcommon breaks their fortran/c++/.. at the same time is very likely to be close to zero. They can adapt the flags in a different way.

@fweimer @law For the record, I'm not quite sure I understand what are your opinions on this (maybe I'm just tired). Could you please explicitly state: "This needs to be added to __global_compiler_flags" or "this needs to be added to optflags"?

Sorry I wasn't clear. I support adding this to optflags or global_compiler_flags. That gives better coverage than just build_cflags. Based on what I've seen in .spec files optflags is probably the best choice.

The choice was build_cflags vs optflags. Due to the way optflags is defined (via rpmrc), the sane way to add a flag on all architectures is to update __global_compiler_flags (which is referenced from rpmrc). I agree with Jeff that we need to changeoptflags (to make legacy packages work with as little hassle with possible).

@churchyard Does this answer your question?

Pull-Request has been merged by churchyard

a month ago