Mark fread's init function as static #6328
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
the trace you gave doesn't actually show any errors, can you please be sure to include the full relevant context? |
|
@MichaelChirico sorry, in my haste I didn't explain further. The backtrace above is from a segfault. This occurs whenever trying to read a CSV file with Making the function as static ensures its usage isn't dynamically resolved, and doesn't depend on other libraries already loaded in the process. |
|
@MichaelChirico for more context, now that I'm at my laptop: |
|
Cannot reproduce on my system. Could you also provide the output of |
IIUC this is .so is not loaded by R (?): libsci_gnu_82_mpi.so and hitting this type of issue IME only happens under certain build environments... |
|
@ben-schwen This is not easily reproducible because it requires a certain context for it to occur. Before continuing with the rest of the message, I'm curious whether there's any objection to the change itself. Even if this wasn't causing issues (it is), marking private functions within a module as The key insight to the problem is that the the segfault trace shows the This is happening in an HPC cluster with SuSE Linux Enterprise Server 15 SP4. The R binary is linked against Cray's Turning on I've been trying locally to reproduce this with a dummy, minimal library using a mix of I can also confirm that in the original system the proposed patch resolves the issue. For completeness, in case it's helpful, this is the |
for myself, I am just not deeply familiar with any tradeoffs presented by the choice to mark the routine OTOH, "namespacing" the function as 'fread_init' instead of the generic 'init' would work without needing to consider whether |
|
Another detail that clarifies things further: not only the I'm not sure how that dependency is put there, because when compiling the package I can't see any references there: @MichaelChirico yes, |
|
TBF there are only two clashing routines on all GitHub: https://github.com/search?q=lang%3Ac+%2F%5Cbfread_init%2F&type=code and this is the first time the issue comes up in 6 years: so I think it's unlikely it'll cause issues. anyway I'm swayed that |
The function isn't used elsewhere, and making it publicly accessible opens the door for runtime linking issues -- where the function is served by other libraries exposing the same function. This was seen in a HPC cluster with software built with spack: 0 0x00001555513d8ce0 in init () from /opt/cray/pe/lib64/libsci_gnu_82_mpi.so.5 1 0x00001555433f46ba in parse_double_extended (...) at fread.c:819 2 0x00001555433f3e97 in detect_types (...) at fread.c:1203 3 0x00001555433f7959 in freadMain (...) at fread.c:1852 4 0x00001555433fd84d in freadR (...) at fRead.c:217 Signed-off-by: Rodrigo Tobar <[email protected]>
|
Thanks @MichaelChirico, I've added an entry under NOTES pointing to this PR. Thanks for the time and consideration! |
MichaelChirico
approved these changes
Jul 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The function isn't used elsewhere, and making it publicly accessible opens the door for runtime linking issues -- where the function is served by other libraries exposing the same function. This was seen in a HPC cluster with software built with spack:
Marking it as
staticprevents these issues.