Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

superenv: disable ConstraintEliminationPass on Xcode 16 #18620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This can result in build failures (e.g. OpenJDK) and silently miscompile
some formulae (e.g. Vim).

See Homebrew/homebrew-core#195325.

@carlocab carlocab requested a review from Bo98 October 24, 2024 03:33
This can result in build failures (e.g. OpenJDK) and silently miscompile
some formulae (e.g. Vim).

See Homebrew/homebrew-core#195325.
@carlocab carlocab changed the title superenv: disable ConstraintEliminiationPass on Xcode 16 superenv: disable ConstraintEliminationPass on Xcode 16 Oct 24, 2024
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @carlocab!

@fxcoudert
Copy link
Member

fxcoudert commented Oct 24, 2024

Isn't that a super heavy-handed fix? We are aware of two formulas where this bug triggers: vim and openjdk. Compilers will always have various slight optimization bugs, and I think it's a bad road to expect to fix them across the board. There is no evidence that this is a very high-profile or high-impact issue at this stage.

@fxcoudert
Copy link
Member

To be clear about the approach I recommend: I suggest enabling the flag for vim and openjdk, and now that the issue is well-identified (thanks to great work on that issue page), see if other reports come in that appear similar.

@carlocab
Copy link
Member Author

Maybe, but the way I see it is: we are trading off between the risk of miscompilation vs the risk of missed optimisations. One is much worse than the other.

If that's not actually the tradeoff that we're making, then do correct me, but once you agree that that's the tradeoff then it's hard not to conclude that you'd rather potentially miss some optimisations than potentially miscompile software.

As for compilers always having slight optimisation bugs: this is true, but "optimisation bugs" come in many different forms, and not all of those forms result in miscompilation. (Some, maybe most, just lead in code that isn't really faster.) Wherever we're aware of any that result in miscompilation and can do something about it, we probably should.

@fxcoudert
Copy link
Member

I don't think it's just missed optimization, these passes are not strongly tested independently (i.e., most code and most tests just specify one -O value) so there is potential risk that we trigger other optimization bugs by forcing an unusual combination of passes. For the majority of software, where there is no known problem, I'd rather ship with the most tested configuration (including the one that Apple themselves use to compile their own OS and system).

I think the benefit/risk ratio would be different if we had a lot of formulas that crashed or otherwise misbehaved, but we have deployed Sonoma bottles for two months and so far we have found issues in only two cases: openjdk bootstrap, and one very specific code path in vim.

@MikeMcQuaid
Copy link
Member

I suggest enabling the flag for vim and openjdk, and now that the issue is well-identified (thanks to great work on that issue page), see if other reports come in that appear similar.

This seems reasonable to me 👍🏻

@fxcoudert
Copy link
Member

This seems reasonable to me 👍🏻

Implemented in vim, and openjdk is currently under testing.

Copy link

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.

@github-actions github-actions bot added the stale No recent activity label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants