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.

Changelog.md Outdated

Bugfixes:
* Assembler: Fix not using a fixed-width type for IDs being assigned to subassemblies nested more than one level away, resulting in inconsistent `--asm-json` output between target architectures.
* General: Fix boost::rational comparison bug with gcc < 14 and C++20 that leads to infinite recursion.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't that GCC<11? Also, we always build with C++20. Boost version is more relevant here.

Suggested change
* General: Fix boost::rational comparison bug with gcc < 14 and C++20 that leads to infinite recursion.
* General: Fix infinite recursion on `boost::rational` comparison affecting compiler binaries built with GCC<14.0 and Boost<1.75.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, we don't need this, do we? We did not fix the bug, we just bumped the versions :)

Perhaps we could just mention this along the version updates below. It could be a single entry mentioning all updated versions and the reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't that GCC<11? Also, we always build with C++20. Boost version is more relevant here.

#16084 states in the title <11 but the issue description mentions <14.

Wait, we don't need this, do we? We did not fix the bug, we just bumped the versions :)

Well. Depends on what you think constitutes a fix, hehe. But yeah, it's not absolutely necessary to put this here, I agree.

Perhaps we could just mention this along the version updates below. It could be a single entry mentioning all updated versions and the reason.

Sounds good!

Copy link
Collaborator

Choose a reason for hiding this comment

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

#16084 states in the title <11 but the issue description mentions <14.

Oh, my bad. The issue mentions both 11 and 14 as does https://osec.io/blog/2025-08-11-compiler-bug-causes-compiler-bug, but rereading them now I realized that 11 referred to the version that was used to reproduce the bug, which was our old requirement. So you're right, the more general condition is <14.

But yeah, it's not absolutely necessary to put this here, I agree.

I see you followed my last suggestion to merge it with the version update entry, but it is still there - I think you forgot to remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But wait. GCC 13.3 is still within the <14 range that triggers the bug. Is that intentional?

Technically, this means that you can still run into this bug on Windows if you use the minimum requirements (Boost 1.77, GCC 13.3) and force CMake to use GCC instead of MSVC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see you followed my last suggestion to merge it with the version update entry, but it is still there - I think you forgot to remove it.

Argh.

Comment on lines -1893 to -1894
- 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+.

…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
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.

@clonker clonker force-pushed the fix_rationa_comparison_bug branch from 91ada88 to aeb6a6e Compare September 25, 2025 14:55
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
2 participants