Skip to content

[fortran] fix bug when copying/memmoving string #19236

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Jun 30, 2025

This Pull request:

Changes or fixes:

happens maybe only in some gfortran versions
Maybe related: https://www.tek-tips.com/threads/string-copy.1609892/post-6335242

in mac13 and mac14 CI, the last bytes of chopt were being set to garbage memory, which later led to failures due to wrong options being set. chopt was being set to "px exam"
where choptt was "px " and "exam" was the first chars of the parameter PASSCHAR("example") on the C side in h2root.cxx

mac14 still fails (with another error), but mac13 h2root works now.

mac13: there is still the error in rpath for two tests.
mac15: failures due to runners having different gfortran versions?

happens maybe only in some gfortran versions
in mac13 and mac14 CI, the last bytes of chopt were being set to garbage memory, which later led to failures due to wrong options being set.
chopt was being set to "px exam"
where choptt was "px " and "exam" was the first chars of the parameter PASSCHAR("example") on the C side in h2root.cxx
now that it's fixed on Fortrans side
now that bug is gone
Copy link

github-actions bot commented Jun 30, 2025

Test Results

    19 files      19 suites   3d 12h 6m 28s ⏱️
 3 063 tests  3 061 ✅ 0 💤 2 ❌
56 669 runs  56 667 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 6622027.

♻️ This comment has been updated with latest results.

@hahnjo
Copy link
Member

hahnjo commented Jul 1, 2025

I think instead of guessing around and applying more and more changes, we should carefully understand the problem in depth and then fix it for good

@ferdymercury ferdymercury requested a review from guitargeek July 1, 2025 09:18
@guitargeek
Copy link
Contributor

guitargeek commented Jul 1, 2025

I think instead of guessing around and applying more and more changes, we should carefully understand the problem in depth and then fix it for good

Maybe before even doing that, we should be clear on whether we actually want to support fortran=ON on mac at this point. It was broken and untested for years, and we only made it work for the newer macOS with this PR last week:

But we see now it's still fragile, and the binaries we provide only work when the user installed gfortran in the same way as it's installed on our CI nodes anyway.

IMHO we need to have a clear strategy on which features of ROOT are supported on which platforms, because we don't necessarily need to support everything everywhere. Attempting that is taking a lot of time and effort, and the features that our users use vary greatly depending on the platform they're on. Once we have decided on these things, we can also make the support status more explicit by making certain configurations error out on certain platforms.

For example, on Windows, fortran=ON would still compile, if the user installs a compiler and makes it somehow findable by enable_language(Fortran). But who can guarantee that h2root still works on Windows at this point and doesn't give you garbage result and corrupt your data?

So I agree with @hahnjo that we should not necessarily try to guess our way out of the problems, but I think we should go one step further and have a more general discussion on what we support on which platform. Possibly in the next ROOT team meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants