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

Code coverage user mode regression in 1.2.0 for inlined code #36462

Closed
yuyichao opened this issue Jun 28, 2020 · 3 comments
Closed

Code coverage user mode regression in 1.2.0 for inlined code #36462

yuyichao opened this issue Jun 28, 2020 · 3 comments
Labels
regression Regression in behavior compared to a previous version

Comments

@yuyichao
Copy link
Contributor

yuyichao commented Jun 28, 2020

At some point in 1.2.0 the coverage decision on whether a code is user mode ignores the module the code is inlined from.

Running

mutable struct A
    b::Int
end

@inline function Base.deepcopy_internal(a::A, d::IdDict)
    a2 = A(a.b)
    d[a] = a2
    return a2
end

deepcopy(A(2))

on 1.1.1 produces the same coverage result for user and all mode but on 1.2.0 the user mode misses one constructor call and all lines in deepcopy_internal.

@code_typed deepcopy(A(2)) and @code_llvm deepcopy(A(2)) confirms that the deepcopy_internal method is correctly inlined and that on 1.2.0 with user mode the store for coverage is missing for deepcopy_internal.

@yuyichao yuyichao added the regression Regression in behavior compared to a previous version label Jun 28, 2020
@fredrikekre
Copy link
Member

fredrikekre commented Aug 25, 2020

A similar example that came up on Slack, looks like same issue, and it is still an issue in Julia 1.5 and master (3d04d15):

$ cat src/Hash.jl 
module Hash

struct X end
function Base.hash(::X, u::UInt)
    println("I was called")
    return hash(123, u)
end

end # module

$ cat test/runtests.jl 
import Hash
hash(Hash.X())

$ jlpkg test --coverage
    Testing Hash
Status `/tmp/jl_qmzAe8/Project.toml`
  [574b4e5d] Hash v0.1.0 `/tmp/tmp.GrIRHL3yAz/Hash`
Status `/tmp/jl_qmzAe8/Manifest.toml`
  [574b4e5d] Hash v0.1.0 `/tmp/tmp.GrIRHL3yAz/Hash`
I was called                                          <------ !!!
    Testing Hash tests passed 

$ cat src/Hash.jl.29362.cov 
        - module Hash
        - 
        1 struct X end
        - function Base.hash(::X, u::UInt)
        -     println("I was called")
        -     return hash(123, u)
        - end
        - 
        - end # module

Lines are marked correctly with --code-coverage=all and when the function is marked explicitly as @noinline.

@racinmat
Copy link

vtjnash added a commit that referenced this issue Aug 27, 2020
While this takes a non-trivial amount of space in the system image
(about the same as putting the Method object here, but less than putting
the full edge to the MethodInstance), it is necessary to avoid the
regression #36462 caused by 2e37784.
vtjnash added a commit that referenced this issue Aug 28, 2020
While this takes a non-trivial amount of space in the system image
(about the same as putting the Method object here, but less than putting
the full edge to the MethodInstance), it is necessary to avoid the
regression #36462 caused by 2e37784.
@yuyichao
Copy link
Contributor Author

yuyichao commented Sep 5, 2020

Fixed by #37243. The commit didn't have the correct keyword to close this.

@yuyichao yuyichao closed this as completed Sep 5, 2020
vchuravy pushed a commit that referenced this issue Jan 22, 2021
While this takes a non-trivial amount of space in the system image
(about the same as putting the Method object here, but less than putting
the full edge to the MethodInstance), it is necessary to avoid the
regression #36462 caused by 2e37784.

(cherry picked from commit 42a1d34)
vchuravy pushed a commit that referenced this issue Jan 22, 2021
While this takes a non-trivial amount of space in the system image
(about the same as putting the Method object here, but less than putting
the full edge to the MethodInstance), it is necessary to avoid the
regression #36462 caused by 2e37784.

(cherry picked from commit 42a1d34)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

3 participants