Skip to content
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

Add more C lib methods #49550

Merged
merged 15 commits into from
May 26, 2023
Merged

Add more C lib methods #49550

merged 15 commits into from
May 26, 2023

Conversation

Tokazama
Copy link
Contributor

No description provided.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Can you convert the existing uses first to use these, so we can use these to reduce duplication?

@quinnj
Copy link
Member

quinnj commented Apr 29, 2023

Looks like there's still an import problem in Random?

UndefVarError: `memcpy` not defined
 ```

base/libc.jl Outdated Show resolved Hide resolved
@Tokazama
Copy link
Contributor Author

Just a heads up, I'm still getting some test errors locally.

base/libc.jl Outdated Show resolved Hide resolved
base/libc.jl Outdated Show resolved Hide resolved
@Tokazama Tokazama requested a review from vtjnash May 8, 2023 19:57
@Tokazama
Copy link
Contributor Author

Tokazama commented May 8, 2023

I think I caught all the import and got testing to work locally at this point.

@Tokazama
Copy link
Contributor Author

Tokazama commented May 9, 2023

Should these methods be usable from the native compiler since "array.jl" and "substring.jl" use them or do we want to keep the native compiler as small as possible here?

@Tokazama Tokazama requested a review from vtjnash May 13, 2023 08:37
base/array.jl Outdated Show resolved Hide resolved
base/array.jl Outdated Show resolved Hide resolved
stdlib/Random/src/XoshiroSimd.jl Outdated Show resolved Hide resolved
base/libc.jl Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label May 22, 2023
Tokazama and others added 11 commits May 22, 2023 17:30
Some ccalls remain in order to accomodate bootstraps still
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
The Libc module contains some code that we might not care to have as
part of bootstrapping. However, the C-memory methods are directly called
throughout bootstrapping so these are now defined in a seperate
"cmem.jl" file that is defined in Base then imported into `Libc` for the
public interface.
Tokazama and others added 4 commits May 22, 2023 17:30
pointer_to_objref(::Array) is not same as unsafe_convert(Ptr, ::Array)
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vtjnash vtjnash merged commit f8dd16e into JuliaLang:master May 26, 2023
@Tokazama Tokazama deleted the c-funcs branch May 26, 2023 20:03
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Jun 5, 2023
BioTurboNick added a commit to BioTurboNick/julia that referenced this pull request Jun 22, 2023
@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 30, 2023

@Tokazama, before 1.10 is released shouldn't those ideally be named memcpy!, memset!, memmove!, memcmp!, to conform to Julia naming conversion (even though not used in C)? I don't think we should commit to supporting without !. If that is needed/wanted we should support as synonym, or at least if this is forgotten and 1.10 released, but I would prefer to not have both.

If changed remember to backport to 1.10.

And about the GC_reserve stuff, I'm not sure, but should it be defined in those functions not their callers?

@Tokazama
Copy link
Contributor Author

The methods here are barebones bindings to C and don't keep with the norms in most Julia code. These methods don't reflect Julia conventions in name or design and probably can't fully follow Julia's conventions. For example, I'm not sure how we'd appropriately make memcpy and memmove more "Julian". We could replace memcpy with unsafe_copyto!(dst::Ptr{UInt8}, src::Ptr{UInt8}, n::Integer) but if there might be overlapping memory it would be better for that to be backed by memmove. There's also the question of how we would handle existing methods like Libc.realloc and Libc.free that also change memory when called.

My personal understanding is the code here only exists to help move C functionality into Julia code, not as something people should use under most circumstances. I'm not sure at what point we care more about being consistent with Julia's conventions or reflecting the C method so it's easy to find and use.

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 31, 2023

You have a point with realloc already there. It and these new are mutating, and maybe should be known to (intended) users, at least to all using C (a dying breed). We can keep C's naming, there are pros and cons, but maybe then explicitly document as mutating, and breaking Julia convention? Is it too excessive to do it on all the functions, rather than somewhere globally?

If this where used from Julia code, then some users would be blind to the mutation, so do you support changing; also realloc -> realloc!, but keep older names as a synonym (to not break; still deprecate?)? Maybe only a synonym for realloc, since 1.10 is not stable yet...

@Tokazama
Copy link
Contributor Author

It would probably be a good idea to document that these methods are generally considered unsafe memory operations and also document which ones mutate memory.

If this where used from Julia code, then some users would be blind to the mutation, so do you support changing; also realloc -> realloc!, but keep older names as a synonym (to not break; still deprecate?)? Maybe only a synonym for realloc, since 1.10 is not stable yet...

At this point the syntax is consistent at least within the Libc module itself. If we're going to change any of the syntax here based on mutation and safety then we should probably discuss renaming most of the functions here. Otherwise, the syntax isn't entirely consistent with Julia code or C code.

This might be worth bringing up in triage because it should probably get more input than me.

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 31, 2023

I or you could open an issue or PR. I don't participate in triage, not sure where and when (is it one Slack?). This is merged on 1.10, so I thought time-critical to change. It could even be done without resize!, since 1.10 is not yet released as stable (seemingly will be any day now), but it would be inconsistent (still ok, and in some sense better), so likely not wanted unless all (relevant) changed in Libc, subsequently, i.e. adding synonym.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 1, 2023

There's a triage channel on slack and I think the zoom call is on scheduled+linked on the community calendar

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.

7 participants