Skip to content

[mono] Make mono_inst_name less misleading #83545

Closed
@lambdageek

Description

@lambdageek

Currently mono_inst_name is defined like this:

const char*
mono_inst_name (int op) {
#ifndef DISABLE_LOGGING
	if (op >= OP_LOAD && op <= OP_LAST)
		return (const char*)&opstr + opidx [op - OP_LOAD];
	if (op < OP_LOAD)
		return mono_opcode_name (op);
	g_error ("unknown opcode name for %d", op);
	return NULL;
#else
	g_error ("unknown opcode name for %d", op);
	g_assert_not_reached ();
#endif
}

The problem is that when logging is disabled, this function is uncallable (the #else branch, above). Any call to it will crash the runtime.

On Android, iOS and WASM we build the runtime with DISABLE_LOGGING defined. Below is Android, but wasm and ios are similar:

      <_MonoCMakeArgs Include="-DENABLE_MINIMAL=ssa,logging" />

Why this is bad:

If someone incorrectly leaves in a call to mono_inst_name in a branch that isn't guarded by #ifndef DISABLE_LOGGING, the runtime will crash. We tend to use mono_inst_name in code that is trying to provide additional diagnostics. So something bad has already happened, we try to gather more info, and instead we crash the runtime.

What we should do instead:

Wrap both the declaration and definition of mono_inst_name in #ifndef DISABLE_LOGGING. Turn any use of mono_inst_name that isn't protected by #ifndef DISABLE_LOGGING into a compile-time error that will fail to build the runtime.

That way, we don't get random crashes during unrelated investigations just because we called some diagnostic functionality

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-VM-meta-monogood first issueIssue should be easy to implement, good for first-time contributorshelp wanted[up-for-grabs] Good issue for external contributorsin-prThere is an active PR which will close this issue when it is merged

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions