Libdl.dlclose incorrectly assumes 0 is success on all platforms

[jdjohnston]

Libdl.dlclose returns true iff the return of the lower-level ccall is zero. However, Windows uses the opposite convention for FreeLibrary (which is what dlload.c/jl_dlclose calls on WIndows): success is non-zero.

Maybe Libdl.dlclose could use something like:

sts = ccall(:jl_dlclose, ...)
if is_windows()
  sts != 0
else
  sts == 0
end

I also noted that while Libdl.dlclose is used in test/libdl.jl, it is never tested.

Maybe also jl_dlclose should, for Windows
if (!handle) return 0;

[vtjnash]

I suppose we just assume that there's very little you can do to recover from an error during cleanup. Anyways, can you submit a PR to change the return value of jl_dlclose on Windows to be consistent with dlclose?

[staticfloat]

In case you're not sure where that is, Jameson is asking you to make this line do the right thing. :)

[vtjnash]

Ah right. I was assuming jdjohnston had already found that from the quality of the report, but should probably try to avoid being as cryptic.

[jdjohnston]

My initial thot was that it would be good to have jl_dlclose behave like the underlying OS system calls and account for the difference in Libdl.dlclose. But there really isn't enough benefit to that to go forward with that approach. Besides a one character addition to jl_dlclose would be simpler, more elegant, more efficient, more ... :)

PR should be up later today or tomorrow at the latest. (I'm running out of 'Net time today.) Something this simple and already discussed doesn't need to marked WIP, right?

[staticfloat]

Something this simple and already discussed doesn't need to marked WIP, right?

Right, feel free to just open the PR directly, and ping myself or Jameson for a merge.

[jdjohnston]

@vtjnash @staticfloat, Please consider PR #25175.

One note: besides the 1-character fix in dlload.c, I also added a test to test/libdl.jl. Since Libdl.dlclose is expected to return different values depending on whether it succeeded or not, it makes sense to have a unit test check for those values. If such a test had existed before, it would have failed on Windows and there would have been no need for my bug report. :) Having a unit test seems like protection against regression.

I realize that a unit test wasn't requested and if need be, I'll gladly retract it from the PR.