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

fix === when encountering null pointer #44749

Merged
merged 7 commits into from
Mar 29, 2022

Conversation

Moelf
Copy link
Contributor

@Moelf Moelf commented Mar 25, 2022

closes #44712

src/builtins.c Outdated Show resolved Hide resolved
@vtjnash vtjnash added backport 1.8 Change should be backported to release-1.8 backport 1.6 Change should be backported to release-1.6 labels Mar 25, 2022
@KristofferC
Copy link
Member

Possible to make a test for this since it is clearly something that is lacking?

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2022

julia> struct AB
       a::Some{Any}
       b::Int
       AB() = new()
       end

julia> unsafe_load(Ptr{AB}(pointer(Int[0,1]))) === unsafe_load(Ptr{AB}(pointer(Int[0,2])))
true

julia> unsafe_load(Ptr{AB}(pointer(Int[0,1]))), unsafe_load(Ptr{AB}(pointer(Int[0,2])))
(AB(#undef, 1), AB(#undef, 2))

@Moelf
Copy link
Contributor Author

Moelf commented Mar 25, 2022

that's a good test, it returns true even in 1.7...

@Moelf
Copy link
Contributor Author

Moelf commented Mar 25, 2022

Error seems unrelated

test/core.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

I rebased it to see if that makes CI happier.

Moelf and others added 5 commits March 27, 2022 23:42
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@Moelf Moelf force-pushed the fix_triple_equal branch from c47ed76 to fa22c34 Compare March 28, 2022 03:43
@KristofferC KristofferC mentioned this pull request Mar 28, 2022
22 tasks
@KristofferC
Copy link
Member

KristofferC commented Mar 28, 2022

Looks like there is still a bug here somewhere. Minimizing the error in CI a bit we get:

using Base.BinaryPlatforms
using Base.Experimental

begin
    Experimental.@force_compile
    p1 = Platform("x86_64", "linux"; julia_version=v"1.5.0")
    p2 = Platform("x86_64", "linux"; julia_version=v"1.6.0")
    @show tags(p1)["julia_version"]
    @show tags(p2)["julia_version"]
end

If this is run with the compilation forced it outputs:

(tags(p1))["julia_version"] = "1.5.0"
(tags(p2))["julia_version"] = "1.5.0"

otherwise

(tags(p1))["julia_version"] = "1.5.0"
(tags(p2))["julia_version"] = "1.6.0"

Breaking out the version arguments into separate variables makes the behaviour go away.

@Moelf
Copy link
Contributor Author

Moelf commented Mar 28, 2022

Is it related to this PR somehow?

@KristofferC
Copy link
Member

Presumably in some way since it doesn't happen on e.g the master branch.

@Moelf
Copy link
Contributor Author

Moelf commented Mar 28, 2022

julia> begin
           Experimental.@force_compile
           p2 = Platform("x86_64", "linux"; julia_version=v"1.6.0")
           @show tags(p2)["julia_version"]
       end
(tags(p2))["julia_version"] = "1.6.0"
"1.6.0"

eh, I don't understand, feels like something to do with compilation / @pureness stuff rather than === in this PR; because if you switch order:

julia> begin
           Experimental.@force_compile
           p2 = Platform("x86_64", "linux"; julia_version=v"1.6.0")
           p1 = Platform("x86_64", "linux"; julia_version=v"1.5.0")
           @show tags(p1)["julia_version"]
           @show tags(p2)["julia_version"]
       end
(tags(p1))["julia_version"] = "1.6.0"
(tags(p2))["julia_version"] = "1.6.0"
"1.6.0"

src/builtins.c Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 28, 2022
@KristofferC
Copy link
Member

Would be nice to have a dedicated test for the latest found problem. Maybe you can cook up one more @vtjnash?

@Moelf
Copy link
Contributor Author

Moelf commented Mar 28, 2022

I think I can cook one up today

@DilumAluthge DilumAluthge added needs tests Unit tests are required for this change and removed merge me PR is reviewed. Merge when all tests are passing labels Mar 28, 2022
@Moelf
Copy link
Contributor Author

Moelf commented Mar 28, 2022

it's harder than I thought, I try to make two immutable structs both containing non-null pointers but are different, I tried something like this but it segfaults:

julia> let a  = Int[1, 1], b = Int[2, 1]
           GC.@preserve a b begin
               unsafe_load(Ptr{N44712}(pointer(a))) === unsafe_load(Ptr{N44712}(pointer(b)))
           end
       end

@vtjnash
Copy link
Member

vtjnash commented Mar 28, 2022

neither of those contain a null pointer

@vtjnash
Copy link
Member

vtjnash commented Mar 28, 2022

What you were breaking is the much more trivial case of some pointer followed by some value:

(("", 0),) === (("", 1),)

normally we compile this, so we don't observe often if it broke

@Moelf
Copy link
Contributor Author

Moelf commented Mar 29, 2022

The windows test failure is unrelated (I checked the CI a bunch of other PRs test also failing)

@KristofferC KristofferC merged commit 1a7355b into JuliaLang:master Mar 29, 2022
KristofferC pushed a commit that referenced this pull request Mar 29, 2022
@KristofferC KristofferC mentioned this pull request Mar 29, 2022
67 tasks
@Moelf Moelf deleted the fix_triple_equal branch March 31, 2022 20:14
@KristofferC KristofferC mentioned this pull request May 16, 2022
45 tasks
KristofferC pushed a commit that referenced this pull request May 23, 2022
KristofferC pushed a commit that referenced this pull request May 23, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jul 6, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in === (caused by #43658?)
4 participants