Skip to content

[h2root] explore/circumvent if occassional memory corruption issue stems from substring replacement #18234

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

Merged
merged 3 commits into from
Jun 25, 2025

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Apr 2, 2025

This Pull request:

Changes or fixes:

By adding a space, we create an extra char that, on Fortran's side will be substituted by c, rather than appending to the (temporary passed as argument C-)string.

Related discussion: https://github.com/root-project/root/pull/15915/files

Fixes #14155

Copy link

github-actions bot commented Apr 2, 2025

Test Results

    20 files      20 suites   3d 18h 32m 0s ⏱️
 3 045 tests  3 042 ✅ 0 💤  3 ❌
59 238 runs  59 226 ✅ 0 💤 12 ❌

For more details on these failures, see this check.

Results for commit e06b2ec.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Apr 2, 2025
@dpiparo dpiparo requested a review from couet April 2, 2025 19:03
@ferdymercury ferdymercury added this to the 6.38.00 milestone Apr 22, 2025
@ferdymercury ferdymercury reopened this Jun 13, 2025
@ferdymercury ferdymercury marked this pull request as ready for review June 13, 2025 22:21
@ferdymercury ferdymercury requested a review from silverweed June 15, 2025 01:58
@pcanal pcanal closed this Jun 17, 2025
@pcanal pcanal reopened this Jun 17, 2025
@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Jun 17, 2025

@pcanal @hageboeck @couet @guitargeek
In MacOS, I am seeing Library not loaded: @rpath/libgfortran.5.dylib and ld: warning: ignoring duplicate libraries: '-lemutls_w', '-lgcc', '-lgfortran', '-lquadmath'
Do you know if this has something to do with incremental building?
Or is it an issue with CMake not properly setting the rpath?
Maybe related: scipy/scipy#19387 (comment)

@pcanal pcanal closed this Jun 18, 2025
@pcanal pcanal reopened this Jun 18, 2025
@pcanal
Copy link
Member

pcanal commented Jun 18, 2025

Outstanding asan diagnostics:

We should open a issue to record those.

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM.

@ferdymercury
Copy link
Collaborator Author

LGTM.

What about the libgfortran dyld failure on mac?

@ferdymercury
Copy link
Collaborator Author

Outstanding asan diagnostics:

We should open a issue to record those.

added to #14682

@pcanal
Copy link
Member

pcanal commented Jun 18, 2025

What about the libgfortran dyld failure on mac?

We indeed need to 'fix' that part before merging.

@couet
Copy link
Member

couet commented Jun 23, 2025

@ferdymercury

@pcanal @hageboeck @couet @guitargeek In MacOS, I am seeing Library not loaded: @rpath/libgfortran.5.dylib

I see this message on the CI build for instance on macphsft31. I checked on that one and gfortran seems properlly installed:

sftnight@macphsft31 ~ % which gfortran                                      
/usr/local/bin/gfortran
sftnight@macphsft31 ~ % ls -l /usr/local/lib/*for*
lrwxr-xr-x  1 root  wheel  43 Mar 20  2024 /usr/local/lib/libgfortran.5.dylib -> /usr/local/gfortran/lib/libgfortran.5.dylib
sftnight@macphsft31 ~ % ls -l /usr/local/gfortran/lib/libgfortran.5.dylib
-rwxr-xr-x  1 root  wheel  1847232 Nov 18  2022 /usr/local/gfortran/lib/libgfortran.5.dylib
sftnight@macphsft31 ~ % 

I checked the other Macs : It is the same

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Jun 24, 2025

@pcanal @hageboeck @couet @guitargeek In MacOS, I am seeing Library not loaded: @rpath/libgfortran.5.dylib

I see this message on the CI build for instance on macphsft31. I checked on that one and gfortran seems properlly installed:

sftnight@macphsft31 ~ % which gfortran                                      
/usr/local/bin/gfortran
sftnight@macphsft31 ~ % ls -l /usr/local/lib/*for*
lrwxr-xr-x  1 root  wheel  43 Mar 20  2024 /usr/local/lib/libgfortran.5.dylib -> /usr/local/gfortran/lib/libgfortran.5.dylib
sftnight@macphsft31 ~ % ls -l /usr/local/gfortran/lib/libgfortran.5.dylib
-rwxr-xr-x  1 root  wheel  1847232 Nov 18  2022 /usr/local/gfortran/lib/libgfortran.5.dylib
sftnight@macphsft31 ~ % 

I checked the other Macs : It is the same

Could it be that this related to #19134 ?

@guitargeek should I try to apply your #19135 here?

@guitargeek
Copy link
Contributor

I don't think adding something hardcoded to the RPATH is a good idea here, because on the users system the libfortran.so might be in a different place. I think the solution is to just set the paths for the test environment correctly:

#19174

But let's see if that actually works 😆

@ferdymercury ferdymercury changed the title [fortran] explore if occassional memory corruption issue stems from substring replacement [h2root] explore/circumvent if occassional memory corruption issue stems from substring replacement Jun 25, 2025
[nfc] comment on how to enable warnings and sanitizer to debug some asan issues that are still present in h2root
…lacement

by adding a space, we create an extra char that will be substituted by c, rather than appending to the string
@guitargeek guitargeek merged commit 2f38cf0 into root-project:master Jun 25, 2025
23 of 24 checks passed
@ferdymercury ferdymercury deleted the h2rootfix branch June 25, 2025 20:40
Comment on lines +315 to +319
// "px" is a string being changed to "pxc" in the fortran side; since it's a temporary on C's side,
// we need a buffer of at least 3 chars on Cs side, too. Predefine it as a extra variable "px " since that way
// the space will be replaced by 'c' on the preallocated memory, instead of appending a new char (new memory) to "px".
auto opt = PASSCHAR("px ");
hropen(lun,PASSCHAR("example"),PASSCHAR(file_in),opt,record_size,ier,7,strlen(file_in),2);
Copy link
Member

Choose a reason for hiding this comment

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

This explanation does not make sense to me because, as far as I can see, the string passed into HROPEN is copied into a character array of length 8:

CHARACTER*8 CHOPT
CHOPT=CHOPTT

Appending a space should not change things; if it does, it's either moving or hiding another problem...

Copy link
Collaborator Author

@ferdymercury ferdymercury Jun 26, 2025

Choose a reason for hiding this comment

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

Yes, it's surprising that march_native no longer crashes with this fix.
I have no idea about Fortran, but, maybe the problem is that the assumed length of CHOPTT in Fortran is off by one, in the sense that it also counts the null-terminator as part of the length, so adding a space forces the copy to work correctly?

Copy link
Collaborator Author

@ferdymercury ferdymercury Jun 26, 2025

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@ferdymercury ferdymercury Jun 30, 2025

Choose a reason for hiding this comment

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

@hahnjo it's confirmed. I added this debug line:

      CHOPT=CHOPTT
      print*, 'HROPEN0: "',CHOPT,'"',LEN(CHOPTT)

And I get:

On mac15 and mac-beta:
HROPEN0: "px " 2

On mac13 and mac14:
HROPEN0: "px exam" 2

which is the sum of "px" and "example" separate arguments in C.

The question now is if this is a gfortran-version-compiler-bug, or if it's an undefined behavior, so we need to fix it by hand.

If I add:

      DO 11 I=LEN(CHOPTT)+1,8
         CHOPT(I:I) = ' '
  11  CONTINUE

Then it seems to work fine in mac13, but not on mac14. See https://github.com/root-project/root/actions/runs/15984458534/job/45085965269?pr=19191

Maybe related: https://www.tek-tips.com/threads/string-copy.1609892/post-6335242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roottest.root.hist.h2root Fails always on march=native builds and sporadically on all other platforms
6 participants