Brew shims override `-O0` in CGo code

SMillerDev commented 1 year ago

`brew doctor` output

Please note that these warnings are just used to help the Homebrew maintainers
with debugging if you file an issue. If everything you use Homebrew for is
working fine: please don't worry or file an issue; just ignore this. Thanks!

Warning: Some installed formulae are deprecated or disabled.
You should find replacements for the following formulae:
  jsonschema
  php@7.4
  php@8.0

Warning: You have uncommitted modifications to Homebrew/homebrew-cask.
If this is a surprise to you, then you should stash these modifications.
Stashing returns Homebrew to a pristine state but can be undone
should you later need to do so for some reason.
  cd /opt/homebrew/Library/Taps/homebrew/homebrew-cask && git stash -u && git clean -d -f

Uncommitted files:
   M Casks/virtualbox.rb

Warning: Some taps are not on the default git origin branch and may not receive
updates. If this is a surprise to you, check out the default branch with:
  git -C $(brew --repo homebrew/core) checkout master

Verification

[X] My "brew doctor output" above says Your system is ready to brew. and am still able to reproduce my issue.
[X] I ran brew update twice and am still able to reproduce my issue.
[X] This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

`brew config` output

HOMEBREW_VERSION: 4.0.3-45-gd877203
ORIGIN: https://github.com/Homebrew/brew
HEAD: d87720333632459964ebce204fd33ecb377598d6
Last commit: 3 hours ago
Core tap origin: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 1f8ea2bbb8667399109a411072b71ae861f65c01
Core tap last commit: 35 minutes ago
Core tap branch: fix/libarchive/pkg_config
Core tap JSON: 22 Feb 15:42 UTC
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_CASK_OPTS: []
HOMEBREW_DEVELOPER: set
HOMEBREW_EDITOR: nano
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 10
HOMEBREW_NO_ENV_HINTS: set
Homebrew Ruby: 2.6.10 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: 10-core 64-bit arm_firestorm_icestorm
Clang: 14.0.0 build 1400
Git: 2.39.2 => /opt/homebrew/bin/git
Curl: 7.86.0 => /usr/bin/curl
macOS: 13.2-arm64
CLT: 14.2.0.0.1.1668646533
Xcode: 14.2 => /Applications/Xcode_14.2.app/Contents/Developer
Rosetta 2: false

What were you trying to do (and why)?

Build ethereum in https://github.com/Homebrew/homebrew-core/pull/123337

What happened (include all command output)?

Build failed without explicitly setting ENV.O0

What did you expect to happen?

The C optimisations we do for normal compilation not to apply to Go.

Step-by-step reproduction instructions (by running `brew` commands)

Remove ENV.O0 if OS.linux? from the ethereum formula.
brew install -s ethereum

MikeMcQuaid commented 1 year ago

The C optimisations we do for normal compilation not to apply to Go.

I don't agree with this? It seems reasonable and desirable for these to apply to CGo.

SMillerDev commented 1 year ago

I don't agree with this? It seems reasonable and desirable for these to apply to CGo.

It seems from https://github.com/golang/go/issues/14669 that Go can't deal with optimisations.

MikeMcQuaid commented 1 year ago

It seems from golang/go#14669 that Go can't deal with optimisations.

Is this the case in literally every formula we have that uses CGo or just this one/a few?

carlocab commented 1 year ago

It seems to only affect four formulae.

This happens when Go calls the function gccErrors, which runs the C compiler and returns the errors from stderr.

// gccErrors runs gcc over the C program stdin and returns
// the errors that gcc prints. That is, this function expects
// gcc to fail.
func (p *Package) gccErrors(stdin []byte, extraArgs ...string) string {

Passing anything other than -O0 can break parsing of the error messages, which is why Go filters out the -O flags and then passes -O0 towards the end of the command.

Our compiler shim undoes what Go is doing here, and that seems to cause problems for some formulae. It's fixed when we set ENV.O0, but that just ends up adding -O0 to every invocation of the compiler, rather than only in the instances where it is needed (i.e. when the compiler is invoked by gccErrors).

MikeMcQuaid commented 1 year ago

Cool, thanks @carlocab. Yeh, that feels like it's worth a fix in the shim for. I don't think we should unconditionally disable the shim for all Go or CGo formula.

github-actions[bot] commented 1 year ago

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

carlocab commented 1 year ago

I was looking at this, and I don't think there's a way to fix this (i.e. pass -O0 if and only if the compiler is called by gccErrors) without patching go.

MikeMcQuaid commented 1 year ago

Thanks for checking this out @carlocab. How about we just use ENV.O0 or similar in def install for this?

carlocab commented 1 year ago

That's what we're already doing for some formulae, because we see build failures for those formulae. I guess the concern is that we might be silently miscompiling others we don't currently use ENV.O0 on. But I have no idea how likely this is -- I don't really use Go so I know very little about how the Go compiler works. Maybe @Bo98, @alebcay, or @issyl0 would know more?

If we don't want to patch Go, an alternative is to get rid of all -O flag refurbishment/injection specifically when go is the one calling the C compiler (perhaps implemented via a go shim). That'll expose more of the refurbishment process to being interfered with by upstream build scripts, though. (Here's an example.)

MikeMcQuaid commented 1 year ago

I guess the concern is that we might be silently miscompiling others we don't currently use ENV.O0 on.

I feel like we'd be getting issues if this were the case?

If we don't want to patch Go, an alternative is to get rid of all -O flag refurbishment/injection specifically when go is the one calling the C compiler (perhaps implemented via a go shim).

This feels overkill to me given we can just selectively use ENV.O0 when needed?

github-actions[bot] commented 1 year ago

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

Homebrew / brew