Skip to content

Fix GCExt test #47699

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.

Sign up for GitHub

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

Merged
vchuravy merged 6 commits into from
Nov 27, 2022
Merged

Fix GCExt test #47699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d30defc
Fix GCExt test with resizable threadpool
vchuravy Nov 24, 2022
0f0fc2b
Add test/gcext to out-of-tree
vchuravy Nov 25, 2022
d891452
Revert "Fix GCExt test with resizable threadpool"
vchuravy Nov 25, 2022
ac544e1
Disable gcext test that uses jl_gc_internal_obj_base_ptr
vchuravy Nov 25, 2022
d8234ea
fixup test/gcext
vchuravy Nov 25, 2022
cd866fd
fixup test
vchuravy Nov 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ all: debug release
# sort is used to remove potential duplicates
DIRS := $(sort $(build_bindir) $(build_depsbindir) $(build_libdir) $(build_private_libdir) $(build_libexecdir) $(build_includedir) $(build_includedir)/julia $(build_sysconfdir)/julia $(build_datarootdir)/julia $(build_datarootdir)/julia/stdlib $(build_man1dir))
ifneq ($(BUILDROOT),$(JULIAHOME))
BUILDDIRS := $(BUILDROOT) $(addprefix $(BUILDROOT)/,base src src/flisp src/support src/clangsa cli doc deps stdlib test test/clangsa test/embedding test/llvmpasses)
BUILDDIRS := $(BUILDROOT) $(addprefix $(BUILDROOT)/,base src src/flisp src/support src/clangsa cli doc deps stdlib test test/clangsa test/embedding test/gcext test/llvmpasses)
BUILDDIRMAKE := $(addsuffix /Makefile,$(BUILDDIRS)) $(BUILDROOT)/sysimage.mk
DIRS += $(BUILDDIRS)
$(BUILDDIRMAKE): | $(BUILDDIRS)
Expand Down
2 changes: 2 additions & 0 deletions src/julia_gcext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ JL_DLLEXPORT int jl_gc_conservative_gc_support_enabled(void);
// external allocations may not all be valid objects and that for those,
// the user *must* validate that they have a proper type, i.e. that
// jl_typeof(obj) is an actual type object.
//
// NOTE: Only valid to call from within a GC context.
JL_DLLEXPORT jl_value_t *jl_gc_internal_obj_base_ptr(void *p);

// Return a non-null pointer to the start of the stack area if the task
Expand Down
20 changes: 10 additions & 10 deletions test/gcext/LocalTest.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ function set_aux_root(n :: Int, x :: String)
return ccall(:set_aux_root, Nothing, (UInt, String), n, x)
end

function internal_obj_scan(p :: Any)
if ccall(:internal_obj_scan, Cint, (Any,), p) == 0
global internal_obj_scan_failures += 1
end
end
# function internal_obj_scan(p :: Any)
# if ccall(:internal_obj_scan, Cint, (Any,), p) == 0
# global internal_obj_scan_failures += 1
# end
# end

global internal_obj_scan_failures = 0
# global internal_obj_scan_failures = 0

for i in 0:1000
set_aux_root(i, string(i))
Expand All @@ -70,12 +70,12 @@ function test()
local stack = make()
for i in 1:100000
push(stack, string(i, base=2))
internal_obj_scan(top(stack))
# internal_obj_scan(top(stack))
end
for i in 1:1000
local stack2 = make()
internal_obj_scan(stack2)
internal_obj_scan(blob(stack2))
# internal_obj_scan(stack2)
# internal_obj_scan(blob(stack2))
while !empty(stack)
push(stack2, pop(stack))
end
Expand All @@ -98,5 +98,5 @@ end
print(gc_counter_full(), " full collections.\n")
print(gc_counter_inc(), " partial collections.\n")
print(num_obj_sweeps(), " object sweeps.\n")
print(internal_obj_scan_failures, " internal object scan failures.\n")
# print(internal_obj_scan_failures, " internal object scan failures.\n")
print(corrupted_roots, " corrupted auxiliary roots.\n")
2 changes: 1 addition & 1 deletion test/gcext/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ $(BIN)/gcext-debug$(EXE): $(SRCDIR)/gcext.c
ifneq ($(abspath $(BIN)),$(abspath $(SRCDIR)))
# for demonstration purposes, our demo code is also installed
# in $BIN, although this would likely not be typical
$(BIN)/LocalModule.jl: $(SRCDIR)/LocalModule.jl
$(BIN)/LocalTest.jl: $(SRCDIR)/LocalTest.jl
cp $< $@
endif

Expand Down
11 changes: 7 additions & 4 deletions test/gcext/gcext-test.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ end
errlines = fetch(err_task)
lines = fetch(out_task)
@test length(errlines) == 0
@test length(lines) == 6
# @test length(lines) == 6
@test length(lines) == 5
@test checknum(lines[2], r"([0-9]+) full collections", n -> n >= 10)
@test checknum(lines[3], r"([0-9]+) partial collections", n -> n > 0)
@test checknum(lines[4], r"([0-9]+) object sweeps", n -> n > 0)
@test checknum(lines[5], r"([0-9]+) internal object scan failures",
n -> n == 0)
@test checknum(lines[6], r"([0-9]+) corrupted auxiliary roots",
# @test checknum(lines[5], r"([0-9]+) internal object scan failures",
# n -> n == 0)
# @test checknum(lines[6], r"([0-9]+) corrupted auxiliary roots",
# n -> n == 0)
@test checknum(lines[5], r"([0-9]+) corrupted auxiliary roots",
n -> n == 0)
end
1 change: 1 addition & 0 deletions test/gcext/gcext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ static size_t gc_alloc_size(jl_value_t *val)

int internal_obj_scan(jl_value_t *val)
{
// FIXME: `jl_gc_internal_obj_base_ptr` is not allowed to be called from outside GC
Copy link
Copy Markdown
Member

@fingolfin fingolfin Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbehrends we already register a task scanner and a root scanner callback in gcext, which exercise jl_gc_internal_obj_base_ptr. So could we just remove internal_obj_scan? (I guess if we did that, a major part of gcext.c and LocalTest.jl would end up dead code and could be removed...)

Or do you think it's still valuable? If so, perhaps the code @vchuravy added here in a previous revision of the PR (and which is still visible in https://github.com/JuliaLang/julia/pull/47407/files) which sets/restores gc_n_threads and gc_all_tls_states, could simply be inserted into internal_obj_scan ?

Copy link
Copy Markdown
Contributor

@rbehrends rbehrends Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the purpose of that part of the test was that all internal object references are properly recognized, so ideally we'd want to keep it around. It's to protect against some future evolution of the GC where the logic might not work for some reason or another.

As I said, I haven't had an opportunity yet to look at exactly what the problem was, but if we could just add code to internal_obj_scan() to make it possible to call jl_gc_internal_obj_base_ptr() from there, that would indeed be the ideal solution.

An alternative would be to check internal_obj_scan() inside mark_stack() and mark_stack_data(). This would require maintaining the failure counter at the C level, but might ultimately be the less hacky solution.

Copy link
Copy Markdown
Member Author

@vchuravy vchuravy Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is more that you are looking at data that is managed by the GC without holding the GC "lock". So the data might be mutated underneath you. So if we could move this to inside the mark_stack that would be better.

if (jl_gc_internal_obj_base_ptr(val) == val) {
size_t size = gc_alloc_size(val);
char *addr = (char *)val;
Expand Down