Skip to content

Conversation

@clonker
Copy link
Member

@clonker clonker commented Aug 20, 2025

Use explicit .operator==() calls to avoid infinite recursion in rational-to-integer comparisons caused by symmetric operator overload resolution bug.

Fixes #16084.

We might potentially want to add a CI job as outlined in #15136 to this PR. Or do it separately.

Adds a CI job that tests on 22.04 which falls into the gcc/boost version category that is problematic. The shipped glibc is too old for current evmone releases, therefore it skips smt and semantic tests.

@clonker clonker force-pushed the fix_rationa_comparison_bug branch 2 times, most recently from 46b9b16 to bf521fe Compare August 20, 2025 17:17
@clonker clonker force-pushed the fix_rationa_comparison_bug branch from bf521fe to e142786 Compare August 27, 2025 08:35
@clonker clonker force-pushed the fix_rationa_comparison_bug branch 4 times, most recently from 4252f0b to 8fd17c5 Compare September 9, 2025 08:10
@clonker clonker requested a review from cameel September 9, 2025 08:17
@clonker clonker changed the title Fix boost::rational comparison bug with gcc < 14 and C++20 Bump boost, gcc, clang for non-windows builds and add CI job with these versions Sep 12, 2025
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

This is testing the wrong CMake version. 3.21.3 is the minimum only on Windows. We support CMake down to 3.13 on Linux. We should also remove the remains of Ubuntu 22.04, which seems to be no longer used after this change.

Other than that mostly minor stuff.

Comment on lines -1893 to -1897
- b_ubu_2204: *requires_nothing
- b_ubu_2204_clang: *requires_nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we no longer need the base_ubuntu2204 definitions and the ubuntu-2204-docker-image Docker image. We should remove these and also its Dockerfile.

Copy link
Collaborator

@cameel cameel Sep 19, 2025

Choose a reason for hiding this comment

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

We could also replace it with a variant of the 2404 image with the lowest supported Clang and GCC pre-installed (by basically integrating your script into its Dockerfile). This way we would not have to install them every time the job runs.

Otherwise we should use save_cache/restore_cache to cache them, but that wouldn't be easy because they throw their stuff around the filesystem.

This is probably better done in a separate PR, because it will result in the images being rebuilt.

Copy link
Member Author

@clonker clonker Sep 22, 2025

Choose a reason for hiding this comment

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

Yep I agree. Otherwise this job is prone to breakage whenever the images are updated. Removing the dockerfile itself would trigger building the images as well right? So let's bundle that with a follow up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

@clonker clonker force-pushed the fix_rationa_comparison_bug branch 2 times, most recently from b06ea0f to 56f8490 Compare September 22, 2025 10:28
@argotorg argotorg deleted a comment from stackenbotten3000 Sep 22, 2025
@clonker clonker force-pushed the fix_rationa_comparison_bug branch 2 times, most recently from cde2d1b to 35b891f Compare September 22, 2025 11:34
@clonker clonker requested a review from cameel September 22, 2025 11:35
@clonker clonker force-pushed the fix_rationa_comparison_bug branch from 35b891f to 498905b Compare September 22, 2025 11:36
@argotorg argotorg deleted a comment from stackenbotten3000 Sep 22, 2025
cameel
cameel previously approved these changes Sep 23, 2025
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

A few more nitpicks, though they are minor and I'm already marking as approved.

cameel

This comment was marked as resolved.

# Boost 1.67 moved container_hash into is own module.
# Boost 1.69 boost::system is header-only and no longer needs to be fetched as component
# Boost 1.70 comes with its own BoostConfig.cmake and is the new (non-deprecated) behavior
find_package(Boost 1.70.0 QUIET REQUIRED COMPONENTS ${BOOST_COMPONENTS})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add an entry for 1.83 to the list above.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough. not sure if this list is really needed in the end but it also doesn't hurt :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is useful to know what the constraints are for being able to use a particular version. Especially when some of them won't necessarily make a build fail. I started listing them after the experience of rebuilding ancient compiler versions for macOS, which would have been much easier if we had the foresight to add such notes on updates. This info is trivial to obtain when you update but disproportionately hard when you have to figure it out by trial and error later.

I guess the older entries become less relevant with time and could be gradually removed, but I also don't see much downside to keeping them. In either case the latest one is the most useful and should be added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, I guess it should be an entry for 1.75, which fixed the bug.

Though 1.83 should be there as well. If we're jumping to 1.83 just because of Trixie and there is no hard blocker to going lower if it turns out we need to for some reason, that's useful information too.

cameel
cameel previously approved these changes Sep 23, 2025
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I think the last thing left is the question about why we're bumping only to GCC 13.3 and not 14+.

@clonker
Copy link
Member Author

clonker commented Sep 23, 2025

I think the last thing left is the question about why we're bumping only to GCC 13.3 and not 14+.

Same reason as for clang: it is the version that comes with Ubuntu 24.04.

…windows builds and of GCC and Clang to 13.3 and 18.1.3, respectively.

Fixes infinite recursion on `boost::rational` comparison affecting compiler binaries built with GCC<14.0 and Boost<1.75.
@clonker clonker force-pushed the fix_rationa_comparison_bug branch from 27c266e to 8c1e489 Compare September 30, 2025 11:51
@clonker clonker merged commit 78627e0 into develop Sep 30, 2025
73 of 74 checks passed
@clonker clonker deleted the fix_rationa_comparison_bug branch September 30, 2025 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The combination of Boost<1.75, GCC<14 and C++20 results in a segfault in rational's equality operator

3 participants