gcc: fix iostream init for Clang on Darwin #135530

saschasc · 2023-07-01T14:01:41Z

Have you followed the guidelines for contributing?
Have you ensured that your commits follow the commit style guide?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This is a fix for gcc. Otherwise this will lead to an immediate segfault when including in when using Clang in combination with libstdc++ 13.1. I've added a guard that the fix needs to be revisited when GCC 13.2 is released.

More details can be found here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110432#c7

Here the fix which will be hopefully be included in GCC 13.2:
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=fe2651affa8c15624188bfd062fb894648743431

There is no need to recreate the bundles. Therefore I suppose that the version number does not need to be increased.

carlocab · 2023-07-01T14:15:05Z

Formula/gcc.rb

+    elsif OS.mac?
+      # libstdc++: Fix iostream init for Clang on Darwin
+      # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110432#c7
+      # https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=fe2651affa8c15624188bfd062fb894648743431
+      if version >= "13.2.0"
+        odie "Review if the fix is still required."
+      else
+        inreplace "/usr/local/opt/gcc@13/include/c++/13/iostream",
+          /^(#if !__has_attribute\(__init_priority__\))$/, '\1 || defined __APPLE__'
+      end


Let's not do this in post_install, please. Particularly since we are calling odie, this should happen as early in the install process as possible.

Probably this could also be done differently without odie. I just wanted to assure that this fix gets removed for the GCC 13.2 release, since this should be fixed then.

Not sure why post_install is not good since this will have no side effect for GCC itself.

Would the bottles needed to be recreated if we put it in install? This is definitely not needed for the fix.

@carlocab To avoid the odie I could also change the if condition to `if version >= "13.1.0" && version < "13.2.0"

if version >= "13.1.0" && version < "13.2.0 inreplace "/usr/local/opt/gcc@13/include/c++/13/iostream", /^(#if !__has_attribute$__init_priority__$)$/, '\1 || defined __APPLE__' end

Would this be better then? I thought to add the odie that this part gets detected more easily and then can be removed. The odie should never happen to a normal user. Only (probably) when a maintainer updates to the GCC >= 13.2.

carlocab · 2023-07-01T14:15:26Z

Formula/gcc.rb

+      if version >= "13.2.0"
+        odie "Review if the fix is still required."
+      else
+        inreplace "/usr/local/opt/gcc@13/include/c++/13/iostream",


We shouldn't hardcode /usr/local here, as that isn't the only prefix that we support.

#{HOMEBREW_PREFIX} would be the correct replacement for it?

carlocab · 2023-07-01T14:18:40Z

Thanks for the PR. I have a few comments, but we'll need approval from @fxcoudert before merging anything.

fxcoudert · 2023-07-02T08:30:08Z

The fix is fine, but we should:

apply it at build-time (not postinstall)
using the upstream patch

m-a-v · 2023-07-02T15:09:36Z

I had communicated with the developers of the corresponding branch which is referenced in this recipe and both the maintainer of this branch and a developer of libstdc++ found the approach ok. I don't know if it is worth to solve this "cleanly" for GCC 13.1. It will not make any difference.

iains/gcc-13-branch#6 (comment)

@fxcoudert Would you or someone else take over?

fxcoudert · 2023-07-02T20:48:04Z

From the linked issue:

As a matter of general policy I am trying to minimise the difference between these branches and upstream

So is Homebrew. We prefer to ship the patch itself. Could you amend your PR to do that?

saschasc · 2023-07-03T12:40:55Z

@fxcoudert Not sure if I will find enough time for it.

What would you expect. A separate patch which will be applied after:

homebrew-core/Formula/gcc.rb

Line 15 in ce44ea3

    
           url "https://raw.githubusercontent.com/Homebrew/formula-patches/5c206c47/gcc/gcc-13.1.0.diff"

If not please communicate exactly how to realize it and if the odie would still be OK to review/remove the patch for GCC >= 13.2. I had seen that the corresponding place is even now slightly different from the version I have in Linux. There is a missing #elif statement in iostream.h which is irrelevant for macOS.

fxcoudert · 2023-07-03T21:58:10Z

The upstream commit might apply directly: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff_plain;h=fe2651affa8c15624188bfd062fb894648743431;hp=d083c8c808cae2c7acb46117374bb047b4c3afce

If not, a patch to formula-patches, ping me on the PR.

odie is not needed: once the patch is not needed any more, applying it will fail if it is not removed.

saschasc · 2023-07-04T05:31:42Z

The upstream commit might apply directly: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff_plain;h=fe2651affa8c15624188bfd062fb894648743431;hp=d083c8c808cae2c7acb46117374bb047b4c3afce

@fxcoudert : So, I would copy the patch content to a file and then apply it after the line 16 in a separate patch command if possible? Correct?

homebrew-core/Formula/gcc.rb

Line 16 in ce44ea3

sha256 "cb4e8a89387f748a744da0273025d0dc2e3c76780cc390b18ada704676afea11"

As far as I've seen it would be patcheable, but I will have to review the diffs.

I just want get exact instructions not to do it twice. Thanks.

carlocab · 2023-07-04T05:47:09Z

Something like this may work:

diff --git a/Formula/gcc.rb b/Formula/gcc.rb
index 87840c02dcb..04ccc7cc9e4 100644
--- a/Formula/gcc.rb
+++ b/Formula/gcc.rb
@@ -15,6 +15,12 @@ class Gcc < Formula
       url "https://raw.githubusercontent.com/Homebrew/formula-patches/5c206c47/gcc/gcc-13.1.0.diff"
       sha256 "cb4e8a89387f748a744da0273025d0dc2e3c76780cc390b18ada704676afea11"
     end
+
+    # Brief comment here explaining what this patch is for and when it can likely be removed.
+    patch do
+      url "https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff_plain;h=fe2651affa8c15624188bfd062fb894648743431;hp=d083c8c808cae2c7acb46117374bb047b4c3afce"
+      sha256 "353ccd8e2c53277f208ad5015e64d4031a6c08eefe5ae0ddbbad22a52a64e100"
+    end
   end
 
   livecheck do

saschasc · 2023-07-04T08:24:57Z

If not, a patch to formula-patches, ping me on the PR.

patching file 'libstdc++-v3/acinclude.m4'
No such line 5679 in input file, ignoring
patching file 'libstdc++-v3/config.h.in'
patching file 'libstdc++-v3/configure'
patching file 'libstdc++-v3/configure.ac'
patching file 'libstdc++-v3/include/std/iostream'
patching file 'libstdc++-v3/src/c++98/ios_base_init.h'

The patch is not applicable from the existing diff file. This diff in the acinclude.m4 is ignored.

@fxcoudert How to proceed?

github-actions · 2023-07-26T00:02:09Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

saschasc · 2023-08-07T08:42:52Z

Since I've not received any further feedback I will not invest more time into it. The fix will implicitly be included in the 13.2 release of gcc.

gcc: fix iostream init for Clang on Darwin

d161f8a

github-actions bot added the long build Needs CI-long-timeout label Jul 1, 2023

saschasc mentioned this pull request Jul 1, 2023

Segmentation fault when using stdlibc++ from gcc 13.1 in combination with clang-16 iains/gcc-13-branch#6

Closed

carlocab requested changes Jul 1, 2023

View reviewed changes

carlocab requested a review from fxcoudert July 1, 2023 14:15

Merge branch 'Homebrew:master' into master

f4f8c75

github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Jul 4, 2023

github-actions bot added the stale No recent activity label Jul 26, 2023

github-actions bot closed this Aug 2, 2023

github-actions bot added the outdated PR was locked due to age label Sep 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcc: fix iostream init for Clang on Darwin #135530

gcc: fix iostream init for Clang on Darwin #135530

saschasc commented Jul 1, 2023

carlocab Jul 1, 2023

saschasc Jul 1, 2023

saschasc Jul 1, 2023

saschasc Jul 1, 2023

carlocab Jul 1, 2023

saschasc Jul 1, 2023

carlocab commented Jul 1, 2023

fxcoudert commented Jul 2, 2023

m-a-v commented Jul 2, 2023

fxcoudert commented Jul 2, 2023

saschasc commented Jul 3, 2023

fxcoudert commented Jul 3, 2023

saschasc commented Jul 4, 2023

carlocab commented Jul 4, 2023

saschasc commented Jul 4, 2023

github-actions bot commented Jul 26, 2023

saschasc commented Aug 7, 2023

gcc: fix iostream init for Clang on Darwin #135530

gcc: fix iostream init for Clang on Darwin #135530

Conversation

saschasc commented Jul 1, 2023

carlocab Jul 1, 2023

Choose a reason for hiding this comment

saschasc Jul 1, 2023

Choose a reason for hiding this comment

saschasc Jul 1, 2023

Choose a reason for hiding this comment

saschasc Jul 1, 2023

Choose a reason for hiding this comment

carlocab Jul 1, 2023

Choose a reason for hiding this comment

saschasc Jul 1, 2023

Choose a reason for hiding this comment

carlocab commented Jul 1, 2023

fxcoudert commented Jul 2, 2023

m-a-v commented Jul 2, 2023

fxcoudert commented Jul 2, 2023

saschasc commented Jul 3, 2023

fxcoudert commented Jul 3, 2023

saschasc commented Jul 4, 2023

carlocab commented Jul 4, 2023

saschasc commented Jul 4, 2023

github-actions bot commented Jul 26, 2023

saschasc commented Aug 7, 2023