[mono] Make mono_inst_name less misleading

[sayeedkhannabil]

Fixes #83545

[SamMonoRT]

LGTM - we'll have to wait for another review too.

[ivanpovazan]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivanpovazan]

@sayeedkhannabil I ran the runtime-extra-platforms pipeline on the CI for you, since it verifies builds for mobile platforms.
The failures seem related.
Could you please check and try to fix the failures?
Let me know if you need help with that.

[sayeedkhannabil]

@ivanpovazan I am looking into the build failures. Is it possible to have a 15 minute chat about the runtime-extra-platforms errors? I am new to automated testing. If you can give me some guideline about this, it would be really great for me.

[ivanpovazan]

@ivanpovazan I am looking into the build failures. Is it possible to have a 15 minute chat about the runtime-extra-platforms errors? I am new to automated testing. If you can give me some guideline about this, it would be really great for me.

Sure, you can reach me out on Discord. My Discord ID is: ivanpovazan

[ivanpovazan]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivanpovazan]

After our offline discussion I reran the failing pipeline since it expired and logs were removed from AzDO.
Here is the link to the build error which has some more information about the cause:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=401505&view=logs&j=67ac41de-1978-5f8d-a67a-2482e49b9998&t=f8ab5ad6-03e7-54dc-430a-d032bae40ebf&l=1334

To reach the link I posted above by yourself, you can follow these steps:

1. Get more information about the failing job:

2. Check more details Azure Pipelines via:

3. The previous step opens up the failed job:

4. By clicking on the failed step - Build product shows the build log with the error message:

The error indicates there is an issue around src/mono/mono/mini/mini.c:605:49

---

Let me know if you need any help or/and if you have any additional questions with resolving the CI/CD errors.

[akoeplinger]

There are a couple changes that need to be fixed, though I'm still wondering why we don't instead change mono_inst_name to return the opcode number as a string in the DISABLE_LOGGING case so we don't need to change any callers

[lambdageek]

There are a couple changes that need to be fixed, though I'm still wondering why we don't instead change mono_inst_name to return the opcode number as a string in the DISABLE_LOGGING case so we don't need to change any callers

That is a good point, although it got me thinking could we do something like this instead:

1. Use the current approach to find all the callers that are misusing the function i.e., calling it when logging is disabled
2. Change the function to have a single responsibility and remove different behaviours controlled by DISABLE_LOGGING feature switch - so it only converts opcode number to string
3. Change the problematic callers found in 1) to assert if needed and handle unknown opcode values ie when function returns NULL

So one problem with changing mono_inst_name to always convert the number to a string is that currently mono_inst_name is non-allocating - it returns a predefined constant string for each opcode. if we changed it to be allocating, we'd have to adjust all the call sites to clean up.

I took a look, and mono_inst_name is used everywhere in print statements of various forms.

So what about doing something like this: make a new printf specifier macro that is "%s" if logging is enabled, and otherwise "%d" (or "0x%04x" or something) if logging is disabled. Concretely:

// in mini.h

#ifndef DISABLE_LOGGING
#define M_PRI_INST "%s"
const char * mono_inst_name(int opcode);
#else
#define M_PRI_INST "%d"
static inline int
mono_inst_name(int opcode)
{
        return opcode;
}
#endif

and then for call sites, change them like this:

- g_assertf (op >= 0 && op < OP_LAST, "unknown opcode %s in function %s", mono_inst_name (op), __FUNCTION__);
+ g_assertf (op >= 0 && op < OP_LAST, "unknown opcode " M_PRI_INST " in function %s", mono_inst_name (op), __FUNCTION__);

That way we still get useful assertions or warnings, but if we disabled logging we'll just get a number instead of the opcode mnemonic.

[lambdageek]

LGTM

[akoeplinger]

I like this approach!