Skip to content

fix duplicate debug instance id in multiple native thread issue #847

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
merged 2 commits into from
Nov 25, 2021
Merged

fix duplicate debug instance id in multiple native thread issue #847

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40df96b
fix duplicate debug instance id in multiple native thread issue
xujuntwt95329 Nov 24, 2021
066b18a
address PR comment: free debug engine if init failed
xujuntwt95329 Nov 25, 2021
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
68 changes: 55 additions & 13 deletions core/iwasm/libraries/debug-engine/debug_engine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,28 @@ typedef struct WASMDebugEngine {
int platform_port;
int process_base_port;
bh_list debug_instance_list;
korp_mutex instance_list_lock;
bool active;
} WASMDebugEngine;

static WASMDebugEngine *g_debug_engine;

static uint32 current_instance_id = 1;

static uint32
allocate_instance_id()
Copy link
Collaborator

@lum1n0us lum1n0us Nov 24, 2021

Choose a reason for hiding this comment

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

how about GCC atomic builtins?

Copy link
Collaborator Author

@xujuntwt95329 xujuntwt95329 Nov 25, 2021

Choose a reason for hiding this comment

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

I‘ve also considered this, but I concern about the compiler compatibility. The GCC atomic builtins are optional features so there may be some compiler who doesn't implement it.

Also, in my opinion, these codes are not on the critical path, the mutex lock should not raise performance issues

@wenyongh how do you think about this?

Copy link
Contributor

@wenyongh wenyongh Nov 25, 2021

Choose a reason for hiding this comment

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

Should not use the optional function which might lead to link error in some platform or compiler. Normally here we should use the API provided by platform layer.

{
uint32 id;

bh_assert(g_debug_engine);

os_mutex_lock(&g_debug_engine->instance_list_lock);
id = current_instance_id++;
os_mutex_unlock(&g_debug_engine->instance_list_lock);

return id;
}

static bool
should_stop(WASMDebugControlThread *control_thread)
{
Expand Down Expand Up @@ -54,7 +71,7 @@ control_thread_routine(void *arg)

control_thread->status = RUNNING;

debug_inst->id = g_debug_engine->debug_instance_list.len + 1;
debug_inst->id = allocate_instance_id();

control_thread->debug_engine = g_debug_engine;
control_thread->debug_instance = debug_inst;
Expand Down Expand Up @@ -137,8 +154,11 @@ wasm_debug_control_thread_create(WASMDebugInstance *debug_instance)
if (!control_thread->server)
goto fail1;

os_mutex_lock(&g_debug_engine->instance_list_lock);
/* create control thread success, append debug instance to debug engine */
bh_list_insert(&g_debug_engine->debug_instance_list, debug_instance);
os_mutex_unlock(&g_debug_engine->instance_list_lock);

wasm_cluster_send_signal_all(debug_instance->cluster, WAMR_SIG_STOP);

return control_thread;
Expand Down Expand Up @@ -177,6 +197,15 @@ wasm_debug_engine_create()
}
memset(engine, 0, sizeof(WASMDebugEngine));

if (os_mutex_init(&engine->instance_list_lock) != 0) {
wasm_runtime_free(engine);
LOG_ERROR("WASM Debug Engine error: failed to init mutex");
Copy link
Contributor

@wenyongh wenyongh Nov 25, 2021

Choose a reason for hiding this comment

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

Should free engine before return NULL

Copy link
Collaborator Author

@xujuntwt95329 xujuntwt95329 Nov 25, 2021

Choose a reason for hiding this comment

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

Done

return NULL;
}

/* reset current instance id */
current_instance_id = 1;

/* TODO: support Wasm platform in LLDB */
/*
engine->control_thread =
Expand All @@ -191,6 +220,16 @@ wasm_debug_engine_create()
return engine;
}

void
wasm_debug_engine_destroy()
{
if (g_debug_engine) {
os_mutex_destroy(&g_debug_engine->instance_list_lock);
wasm_runtime_free(g_debug_engine);
g_debug_engine = NULL;
}
}

bool
wasm_debug_engine_init(char *ip_addr, int platform_port, int process_port)
{
Expand Down Expand Up @@ -230,15 +269,6 @@ wasm_debug_get_engine_active(void)
return false;
}

void
wasm_debug_engine_destroy()
{
if (g_debug_engine) {
wasm_runtime_free(g_debug_engine);
g_debug_engine = NULL;
}
}

/* A debug Instance is a debug "process" in gdb remote protocol
and bound to a runtime cluster */
WASMDebugInstance *
Expand Down Expand Up @@ -276,13 +306,19 @@ wasm_debug_instance_create(WASMCluster *cluster)
static WASMDebugInstance *
wasm_cluster_get_debug_instance(WASMDebugEngine *engine, WASMCluster *cluster)
{
WASMDebugInstance *instance =
bh_list_first_elem(&engine->debug_instance_list);
WASMDebugInstance *instance;

os_mutex_lock(&g_debug_engine->instance_list_lock);
instance = bh_list_first_elem(&engine->debug_instance_list);
while (instance) {
if (instance->cluster == cluster)
if (instance->cluster == cluster) {
os_mutex_unlock(&g_debug_engine->instance_list_lock);
return instance;
}
instance = bh_list_elem_next(instance);
}
os_mutex_unlock(&g_debug_engine->instance_list_lock);

return instance;
}

Expand Down Expand Up @@ -315,7 +351,10 @@ wasm_debug_instance_destroy(WASMCluster *cluster)
if (instance) {
/* destroy control thread */
wasm_debug_control_thread_destroy(instance);

os_mutex_lock(&g_debug_engine->instance_list_lock);
bh_list_remove(&g_debug_engine->debug_instance_list, instance);
os_mutex_unlock(&g_debug_engine->instance_list_lock);

/* destroy all breakpoints */
wasm_debug_instance_destroy_breakpoints(instance);
Expand Down Expand Up @@ -700,12 +739,15 @@ wasm_exec_env_get_instance(WASMExecEnv *exec_env)
WASMDebugInstance *instance = NULL;
bh_assert(g_debug_engine);

os_mutex_lock(&g_debug_engine->instance_list_lock);
instance = bh_list_first_elem(&g_debug_engine->debug_instance_list);
while (instance) {
if (instance->cluster == exec_env->cluster)
break;
instance = bh_list_elem_next(instance);
}

os_mutex_unlock(&g_debug_engine->instance_list_lock);
return instance;
}

Expand Down