Skip to content

Remove unneeded cglobal in cglobal(dlsym(...))#1230

Merged
mkitti merged 1 commit into
JuliaIO:masterfrom
topolarity:ct/fix-cglobal
May 2, 2026
Merged

Remove unneeded cglobal in cglobal(dlsym(...))#1230
mkitti merged 1 commit into
JuliaIO:masterfrom
topolarity:ct/fix-cglobal

Conversation

@topolarity

@topolarity topolarity commented May 1, 2026

Copy link
Copy Markdown
Contributor

The cglobal is not needed here.

We'd like to consider deprecating / removing this cglobal(::Ptr) support in Julia 1.14 (JuliaLang/julia#61707).

@mkitti

mkitti commented May 1, 2026

Copy link
Copy Markdown
Member

ERROR: LoadError: TypeError: in cglobal: first argument not a pointer or valid constant expression, expected Ptr, got a value of type Tuple{Ptr{Nothing}, Symbol}

This does not seem to work...

@mkitti

mkitti commented May 1, 2026

Copy link
Copy Markdown
Member

Do we need a macro here instead?

The `cglobal(::Ptr, ::T)` pattern here is really just a fancy way to
cast to `Ptr{T}` and it is likely to be deprecated soon, so best to
avoid using `cglobal` for this.
@topolarity topolarity changed the title Remove unneeded dlsym in cglobal(dlsym(...)) Remove unneeded cglobal in cglobal(dlsym(...)) May 1, 2026
@topolarity

topolarity commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Ah right this is a dynamic usage, sorry about that.

The error is a bit obtuse, but it means that the argument is not known statically. The new form with Ptr{T}(dlsym(...)) should work (and be equivalent to what was there before).

@mkitti

mkitti commented May 1, 2026

Copy link
Copy Markdown
Member

Kind of. We actually use all of these statically...

HDF5.jl/src/api/types.jl

Lines 385 to 401 in 66ee074

const H5P_OBJECT_CREATE = _read_const(:H5P_CLS_OBJECT_CREATE_ID_g)
const H5P_FILE_CREATE = _read_const(:H5P_CLS_FILE_CREATE_ID_g)
const H5P_FILE_ACCESS = _read_const(:H5P_CLS_FILE_ACCESS_ID_g)
const H5P_DATASET_CREATE = _read_const(:H5P_CLS_DATASET_CREATE_ID_g)
const H5P_DATASET_ACCESS = _read_const(:H5P_CLS_DATASET_ACCESS_ID_g)
const H5P_DATASET_XFER = _read_const(:H5P_CLS_DATASET_XFER_ID_g)
const H5P_FILE_MOUNT = _read_const(:H5P_CLS_FILE_MOUNT_ID_g)
const H5P_GROUP_CREATE = _read_const(:H5P_CLS_GROUP_CREATE_ID_g)
const H5P_GROUP_ACCESS = _read_const(:H5P_CLS_GROUP_ACCESS_ID_g)
const H5P_DATATYPE_CREATE = _read_const(:H5P_CLS_DATATYPE_CREATE_ID_g)
const H5P_DATATYPE_ACCESS = _read_const(:H5P_CLS_DATATYPE_ACCESS_ID_g)
const H5P_STRING_CREATE = _read_const(:H5P_CLS_STRING_CREATE_ID_g)
const H5P_ATTRIBUTE_ACCESS = _read_const(:H5P_CLS_ATTRIBUTE_ACCESS_ID_g)
const H5P_ATTRIBUTE_CREATE = _read_const(:H5P_CLS_ATTRIBUTE_CREATE_ID_g)
const H5P_OBJECT_COPY = _read_const(:H5P_CLS_OBJECT_COPY_ID_g)
const H5P_LINK_CREATE = _read_const(:H5P_CLS_LINK_CREATE_ID_g)
const H5P_LINK_ACCESS = _read_const(:H5P_CLS_LINK_ACCESS_ID_g)

@mkitti

mkitti commented May 1, 2026

Copy link
Copy Markdown
Member

Would there be any advantage or disadvantage to doing:

julia> macro hdf5_global(ex)
           quote
               unsafe_load(Ptr{UInt64}(cglobal(($ex, HDF5_jll.libhdf5_path))))
           end
       end
@hdf5_global (macro with 1 method)

julia> @hdf5_global :H5T_STD_I16BE_g
0x030000000000004e

@topolarity

Copy link
Copy Markdown
Contributor Author

Yes, that will opt in to Julia's built-in caching, rather than calling dlsym on every load

The macro would be a performance improvement over the status quo, in that case

@mkitti

mkitti commented May 2, 2026

Copy link
Copy Markdown
Member

At the moment, we assign everything to a top-level const, so I guess this results in another layer of caching that gets stored in the precompile cache and may be a bit fragile.

 const H5P_DATASET_ACCESS   = _read_const(:H5P_CLS_DATASET_ACCESS_ID_g) 

If someone switched out the HDF5 library in such a way that does not HDF5.jl to recompile, these constants might be invalid. I will merge this for now and consider refactoring later.

@mkitti mkitti merged commit 51614c2 into JuliaIO:master May 2, 2026
36 checks passed
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.

2 participants