[Flang][runtime] Fix RENAME intrinsic, remove trailing blanks #159123
Conversation
The RENAME intrinsic did not correctly remove trailing spaces from filenames. This PR introduces code to remove trailing blanks as documented by GFortran.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the RENAME intrinsic in the Flang runtime to properly handle trailing spaces in filenames by removing them before performing the rename operation, aligning with GFortran's documented behavior.
- Adds trailing space trimming functionality for both source and destination paths
- Replaces direct rename call with trimmed path versions
- Uses stack allocation for temporary trimmed path buffers
Co-authored-by: Copilot <[email protected]>
| char *pathDst{EnsureNullTerminated( | ||
| path2.OffsetElement(), path2.ElementBytes(), terminator)}; | ||
|
|
||
| // Trim trailing blanks |
There was a problem hiding this comment.
File names in most unix-like filesystems can have trailing spaces. I think it's the caller that should make sure the names are correct.
There was a problem hiding this comment.
I do agree with that sentiment, but this implements the semantics of GFortran with this PR: https://gcc.gnu.org/onlinedocs/gfortran/RENAME.html.
There was a problem hiding this comment.
@kparzysz, note that you can still deal with blank terminated files on UNIX system after this patch by using rename("test_trailing "//char(0), "new_name "//char(0)).
This is consistent not only with gfortran, but also with IFX and nvfortran/classic flang implementations. Given this is a legacy lib3f intrinsic, we must stick to the existing behaviors here.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jeanPerier
left a comment
jeanPerier
left a comment
There was a problem hiding this comment.
Thanks, make sense to me.
Simplification suggestion inlined.
| // Get the raw strings (null-terminated) | ||
| char *pathSrc{EnsureNullTerminated( | ||
| path1.OffsetElement(), path1.ElementBytes(), terminator)}; | ||
| char *pathDst{EnsureNullTerminated( | ||
| path2.OffsetElement(), path2.ElementBytes(), terminator)}; | ||
| char *srcFilePath = pathSrc; | ||
| char *dstFilePath = pathDst; | ||
|
|
||
| // Trim trailing blanks (if string have not been null-terminated) | ||
| if (!IsNullTerminated(path1.OffsetElement(), path1.ElementBytes())) { | ||
| auto srcTrimPos{TrimTrailingSpaces(pathSrc, path1.ElementBytes())}; | ||
| char *srcPathTrim{ | ||
| static_cast<char *>(alloca((srcTrimPos + 1)))}; | ||
| std::memcpy(srcPathTrim, pathSrc, srcTrimPos); | ||
| srcPathTrim[srcTrimPos] = '\0'; | ||
| srcFilePath = srcPathTrim; | ||
| } | ||
| if (!IsNullTerminated(path2.OffsetElement(), path2.ElementBytes())) { | ||
| auto dstTrimPos{TrimTrailingSpaces(pathDst, path2.ElementBytes())}; | ||
| char *dstPathTrim{ | ||
| static_cast<char *>(alloca((dstTrimPos + 1)))}; | ||
| std::memcpy(dstPathTrim, pathDst, dstTrimPos); | ||
| dstPathTrim[dstTrimPos] = '\0'; | ||
| dstFilePath = dstPathTrim; | ||
| } |
There was a problem hiding this comment.
| // Get the raw strings (null-terminated) | |
| char *pathSrc{EnsureNullTerminated( | |
| path1.OffsetElement(), path1.ElementBytes(), terminator)}; | |
| char *pathDst{EnsureNullTerminated( | |
| path2.OffsetElement(), path2.ElementBytes(), terminator)}; | |
| char *srcFilePath = pathSrc; | |
| char *dstFilePath = pathDst; | |
| // Trim trailing blanks (if string have not been null-terminated) | |
| if (!IsNullTerminated(path1.OffsetElement(), path1.ElementBytes())) { | |
| auto srcTrimPos{TrimTrailingSpaces(pathSrc, path1.ElementBytes())}; | |
| char *srcPathTrim{ | |
| static_cast<char *>(alloca((srcTrimPos + 1)))}; | |
| std::memcpy(srcPathTrim, pathSrc, srcTrimPos); | |
| srcPathTrim[srcTrimPos] = '\0'; | |
| srcFilePath = srcPathTrim; | |
| } | |
| if (!IsNullTerminated(path2.OffsetElement(), path2.ElementBytes())) { | |
| auto dstTrimPos{TrimTrailingSpaces(pathDst, path2.ElementBytes())}; | |
| char *dstPathTrim{ | |
| static_cast<char *>(alloca((dstTrimPos + 1)))}; | |
| std::memcpy(dstPathTrim, pathDst, dstTrimPos); | |
| dstPathTrim[dstTrimPos] = '\0'; | |
| dstFilePath = dstPathTrim; | |
| } | |
| auto srcFilePath{SaveDefaultCharacter(path1.OffsetElement(), TrimTrailingSpaces(path1.OffsetElement(), path1.ElementBytes()), terminator)}; | |
| auto dstFilePath{SaveDefaultCharacter(path2.OffsetElement(), TrimTrailingSpaces(path2.OffsetElement(), path2.ElementBytes()), terminator)}; |
(then use srcFilePath.get() and dstFilePath.get() in rename call below).
Unformatted/untested.
You should get the same behavior because CHAR(0) is not a blank, so trimming will have no effect when this is the last character and no special handling for this case is needed.
There was a problem hiding this comment.
Thanks for the suggestion! That is much better than what I had :-) and it seems to work for what I have tested.
| char *pathDst{EnsureNullTerminated( | ||
| path2.OffsetElement(), path2.ElementBytes(), terminator)}; | ||
|
|
||
| // Trim trailing blanks |
There was a problem hiding this comment.
@kparzysz, note that you can still deal with blank terminated files on UNIX system after this patch by using rename("test_trailing "//char(0), "new_name "//char(0)).
This is consistent not only with gfortran, but also with IFX and nvfortran/classic flang implementations. Given this is a legacy lib3f intrinsic, we must stick to the existing behaviors here.
This reverts commit 6827079.
Simplification suggested by @jeanPerier
3 participants
The RENAME intrinsic did not correctly remove trailing spaces from filenames. This PR introduces code to remove trailing blanks as documented by GFortran.