[mono] Make mono_inst_name less misleading

[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

[lambdageek]

/cc @steveisok @SamMonoRT

[lambdageek]

luckily mono_inst_name isn't in the public API. so we can mess with its declaration

[sayeedkhannabil]

Hi, I'm Abu Sayeed Khan, a final year computer science student. I came across this issue, and I think it's a great opportunity for me to learn more about dotnet. I'm interested in working on this issue, and I'd appreciate any guidance or advice you can provide. Thank you!

[sayeedkhannabil]

Is it okay to define a dummy macro to create compilation error? Something like this:

#ifndef DISABLE_LOGGING
const char* mono_inst_name (int op);
#else
#define mono_inst_name(op) _static_assert_disabled_logging()
static inline void _static_assert_disabled_logging(void)
{
    static_assert(0, "mono_inst_name is not available when DISABLE_LOGGING is defined.");
}
#endif

[lambdageek]

I'm interested in working on this issue, and I'd appreciate any guidance or advice you can provide.

Sure, give it a go. Thanks for helping us!

Is it okay to define a dummy macro to create compilation error? Something like this:

#ifndef DISABLE_LOGGING
const char* mono_inst_name (int op);
#else
#define mono_inst_name(op) _static_assert_disabled_logging()
static inline void _static_assert_disabled_logging(void)
{
    static_assert(0, "mono_inst_name is not available when DISABLE_LOGGING is defined.");
}
#endif

I don't think we need a specialized assertion. Just something like this is enough:

/* in mini.h */
....
#ifndef DISABLE_LOGGING
const char *mono_inst_name (int op);
#endif

The C compiler should error if some code in another file tries to call mono_inst_name when it's undefined.

We compile mono as C99, so static_assert is not available to us. (but it's possible to emulate it with C tricks, so that's not really a total dealbreaker).

The actual issue is, I don't think using a static_assert will work here: it will make the definition of _static_assert_disabled_logging fail to compile even if noone calls it. We would need to make mono_inst_name a macro that expands to a some kind of compilation error. Perhaps something like

#define mono_inst_name(op) _STATIC_COMPILE_ERROR("Not available with DISABLED_LOGGING")

#define _STATIC_COMPILE_ERROR(msg) extern void _fail_the_build(int [sizeof(char[-2])] )

if I call mono_inst_name(123), for me clang and gcc both produce an error like this, which is probably good enough:

$ clang -std=c99 -c foo.c -DFAILME
foo.c:19:2: error: array size is negative
        mono_inst_name(123);
        ^~~~~~~~~~~~~~~~~
foo.c:5:28: note: expanded from macro 'mono_inst_name'
#define mono_inst_name(op) _STATIC_COMPILE_ERROR("Not available with DISABLED_LOGGING")
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
foo.c:7:81: note: expanded from macro '_STATIC_COMPILE_ERROR'
#define _STATIC_COMPILE_ERROR(msg) extern void _fail_the_build(int [sizeof(char[-2])] )
                                                                                ^~
1 error generated.
$ gcc -std=c99 -c foo.c -DFAILME
foo.c: In function ‘main’:
foo.c:7:80: error: size of unnamed array is negative
    7 | #define _STATIC_COMPILE_ERROR(msg) extern void _fail_the_build(int [sizeof(char[-2])] )
      |                                                                                ^
foo.c:5:28: note: in expansion of macro ‘_STATIC_COMPILE_ERROR’
    5 | #define mono_inst_name(op) _STATIC_COMPILE_ERROR("Not available with DISABLED_LOGGING")
      |                            ^~~~~~~~~~~~~~~~~~~~~
foo.c:19:9: note: in expansion of macro ‘mono_inst_name’
   19 |         mono_inst_name(123);
      |         ^~~~~~~~~~~~~~

[akoeplinger]

Wouldn't it be easier to change mono_inst_name to return the opcode integer value as string instead in the DISABLE_LOGGING case so we don't need to change every caller?

We also seem to be embedding CIL opcodes in mono_opcode_name still since we don't have DISABLE_LOGGING there.

[SamMonoRT]

Moving this to 9.0.0