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/segfault rai #1

Merged
merged 2 commits into from
Jan 17, 2023
Merged

Fix/segfault rai #1

merged 2 commits into from
Jan 17, 2023

Conversation

udesou
Copy link

@udesou udesou commented Jan 17, 2023

Pull request to fix segfault when running (pending) finalisers.

@udesou udesou merged commit c5be9d9 into mmtk:v1.8.2-RAI Jan 17, 2023
qinsoon pushed a commit to qinsoon/julia that referenced this pull request May 2, 2024
…#51489)

This exposes the GC "stop the world" API to the user, for causing a
thread to quickly stop executing Julia code. This adds two APIs (that
will need to be exported and documented later):
```
julia> @CCall jl_safepoint_suspend_thread(#=tid=mmtk#1::Cint, #=magicnumber=mmtk#2::Cint)::Cint # roughly tkill(1, SIGSTOP)

julia> @CCall jl_safepoint_resume_thread(#=tid=mmtk#1::Cint)::Cint # roughly tkill(1, SIGCONT)
```

You can even suspend yourself, if there is another task to resume you 10
seconds later:
```
julia> ccall(:jl_enter_threaded_region, Cvoid, ())

julia> t = @task let; Libc.systemsleep(10); print("\nhello from $(Threads.threadid())\n"); @CCall jl_safepoint_resume_thread(0::Cint)::Cint; end; ccall(:jl_set_task_tid, Cint, (Any, Cint), t, 1); schedule(t);

julia> @time @CCall jl_safepoint_suspend_thread(0::Cint, 2::Cint)::Cint

hello from 2
  10 seconds (6 allocations: 264 bytes)
1
```

The meaning of the magic number is actually the kind of stop that you
want:
```
// n.b. suspended threads may still run in the GC or GC safe regions
// but shouldn't be observable, depending on which enum the user picks (only 1 and 2 are typically recommended here)
// waitstate = 0 : do not wait for suspend to finish
// waitstate = 1 : wait for gc_state != 0 (JL_GC_STATE_WAITING or JL_GC_STATE_SAFE)
// waitstate = 2 : wait for gc_state != 0 (JL_GC_STATE_WAITING or JL_GC_STATE_SAFE) and that GC is not running on that thread
// waitstate = 3 : wait for full suspend (gc_state == JL_GC_STATE_WAITING) -- this may never happen if thread is sleeping currently
// if another thread comes along and calls jl_safepoint_resume, we also return early
// return new suspend count on success, 0 on failure
```
Only magic number 2 is currently meaningful to the user though. The
difference between waitstate 1 and 2 is only relevant in C code which is
calling this from JL_GC_STATE_SAFE, since otherwise it is a priori known
that GC isn't running, else we too would be running the GC. But the
distinction of those states might be useful if we have a concurrent
collector.

Very important warning: if the stopped thread is holding any locks
(e.g. for codegen or types) that you then attempt to acquire, your
thread will deadlock. This is very likely, unless you are very careful.
A future update to this API may try to change the waitstate to give the
option to wait for the thread to release internal or known locks.
qinsoon pushed a commit to qinsoon/julia that referenced this pull request May 2, 2024
…ang#53631)

This PR validates the input parameters to the Julia LAPACK wrappers, so
that the error messages are more informative.
On nightly
```julia
julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
 ** On entry to DGEEV  parameter number  1 had an illegal value
ERROR: ArgumentError: invalid argument mmtk#1 to LAPACK call
```
This PR
```julia
julia> using LinearAlgebra

julia> LAPACK.geev!('X', 'X', rand(2,2))
ERROR: ArgumentError: argument mmtk#1: jobvl must be one of ('N', 'V'), but 'X' was passed
```

Secondly, moved certain allocations (e.g. in `geevx`) below the
validation checks, so that these only happen for valid parameter values.

Thirdly, added `require_one_based_indexing` checks to functions where
these were missing.
qinsoon pushed a commit to qinsoon/julia that referenced this pull request May 2, 2024
This is an alternative to JuliaLang#53642

The `dom_edges()` for an exit block in the CFG are empty when computing
the PostDomTree so the loop below this may not actually run. In that
case, the right semidominator is the ancestor from the DFSTree, which is
the "virtual" -1 block.

This resolves half of the issue in
JuliaLang#53613:
```julia
julia> let code = Any[
               # block 1
               GotoIfNot(Argument(2), 3),
               # block 2
               ReturnNode(Argument(3)),
               # block 3 (we should visit this block)
               Expr(:call, throw, "potential throw"),
               ReturnNode(), # unreachable
           ]
           ir = make_ircode(code; slottypes=Any[Any,Bool,Bool])
           visited = BitSet()
           @test !Core.Compiler.visit_conditional_successors(CC.LazyPostDomtree(ir), ir, #=bb=mmtk#1) do succ::Int
               push!(visited, succ)
               return false
           end
           @test 2 ∈ visited
           @test 3 ∈ visited
       end
Test Passed
```

This needs some tests (esp. since I don't think we have any DomTree
tests at all right now), but otherwise should be good to go.
qinsoon pushed a commit to qinsoon/julia that referenced this pull request May 2, 2024
…iaLang#53642)

This commit fixes the first problem that was found while digging into
JuliaLang#53613. It turns out that the post-domtree constructed
from regular `IRCode` doesn't work for visiting conditional successors
for post-opt analysis in cases like:
```julia
julia> let code = Any[
               # block 1
               GotoIfNot(Argument(2), 3),
               # block 2
               ReturnNode(Argument(3)),
               # block 3 (we should visit this block)
               Expr(:call, throw, "potential throw"),
               ReturnNode(), # unreachable
           ]
           ir = make_ircode(code; slottypes=Any[Any,Bool,Bool])
           visited = BitSet()
           @test !Core.Compiler.visit_conditional_successors(CC.LazyPostDomtree(ir), ir, #=bb=mmtk#1) do succ::Int
               push!(visited, succ)
               return false
           end
           @test 2 ∉ visited
           @test 3 ∈ visited
       end
Test Failed at REPL[14]:16
  Expression: 2 ∉ visited
   Evaluated: 2 ∉ BitSet([2])
```

This might mean that we need to fix on the `postdominates` end, but for
now, this commit tries to get around it by using the augmented post
domtree in `visit_conditional_successors`. Since the augmented post
domtree is enforced to have a single return, we can keep using the
current `postdominates` to fix the issue.

However, this commit isn't enough to fix the NeuralNetworkReachability
segfault as reported in JuliaLang#53613, and we need to tackle the second issue
reported there too
(JuliaLang#53613 (comment)).
udesou referenced this pull request in udesou/julia Jun 4, 2024
* Change to streaming out the heap snapshot data (#1)

* Streaming the heap snapshot!

This should prevent the engine from OOMing while recording the snapshot!

Now we just need to sample the files, either online, before downloading,
or offline after downloading :)

If we're gonna do it offline, we'll want to gzip the files before
downloading them.

* Allow custom filename; use original API

* Support legacy heap snapshot interface. Add reassembly function.

* Add tests

* Apply suggestions from code review

* Update src/gc-heap-snapshot.cpp

* Change to always save the parts in the same directory

This way you can always recover from an OOM

* Fix bug in reassembler: from_node and to_node were in the wrong order

* Fix correctness mistake: The edges have to be reordered according to the
node order. That's the whole reason this is tricky.

But i'm not sure now whether the SoAs approach is actually an
optimization.... It seems like we should probably prefer to inline the
Edges right into the vector, rather than having to do another random
lookup into the edges table?

* Debugging messed up edge array idxs

* Disable log message

* Write the .nodes and .edges as binary data

* Remove unnecessary logging

* fix merge issues

* attempt to add back the orphan node checking logic

---------

Co-authored-by: Nathan Daly <nathan.daly@relational.ai>
Co-authored-by: Nathan Daly <NHDaly@gmail.com>

* attempt to fix the doc issue for assemble_snapshot

remove unused k_node_number_of_fields from gc-heap-snapshot.cpp

attempt to resolve the savepoint issue on serialize_node

* remove println in take_heap_snapshot to avoid messing up console output in Julia REPL

* rename alloc_type for array buffer in gc-heap-snapshot

* streaming strings directly to avoid cache in memory

dedupling strings for field paths

* address PR comments

---------

Co-authored-by: Nathan Daly <nathan.daly@relational.ai>
Co-authored-by: Nathan Daly <NHDaly@gmail.com>
udesou referenced this pull request in udesou/julia Aug 29, 2024
…aLang#55600)

As an application of JuliaLang#55545, this commit avoids the
insertion of `:throw_undef_if_not` nodes when the defined-ness of a slot
is guaranteed by abstract interpretation.

```julia
julia> function isdefined_nothrow(c, x)
           local val
           if c
               val = x
           end
           if @isdefined val
               return val
           end
           return zero(Int)
       end;

julia> @code_typed isdefined_nothrow(true, 42)
```
```diff
diff --git a/old b/new
index c4980a5c9c..3d1d6d30f0 100644
--- a/old
+++ b/new
@@ -4,7 +4,6 @@ CodeInfo(
 3 ┄ %3 = φ (mmtk#2 => x, #1 => #undef)::Int64
 │   %4 = φ (mmtk#2 => true, #1 => false)::Bool
 └──      goto mmtk#5 if not %4
-4 ─      $(Expr(:throw_undef_if_not, :val, :(%4)))::Any
-└──      return %3
+4 ─      return %3
 5 ─      return 0
 ) => Int64
```
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.

1 participant