Making mono_inst_name less misleading

[sayeedkhannabil]

Wraping both declaration and definition in #ifndef DISABLE_LOGGING.

Fix #83545

[sayeedkhannabil]

@dotnet-policy-service agree

[sayeedkhannabil]

Do I have to add #ifndef DISABLE_LOGGING before all of the function call of mono_inst_name() to pass the tests?

[lambdageek]

This whole #else part can be removed

[lambdageek]

This is an interesting idea. We don't only compile with gcc and clang, so we need to make some macros to make it workable. Also it's better to use __attribute__((unavailable)) when the compiler supports it, because then optimizations that remove a call won't remove the warning.

I did an example on godbolt that seems to work even with old versions of gcc and clang.

Add this to src/mono/mono/utils/mono-compiler.h

#if defined(__clang__)
#define MONO_FUNCTION_UNAVAILABLE(msg) __attribute__((unavailable(msg)))
#elif (__GNUC__ == 12 && __GNUC_MINOR__ >= 1)
#define MONO_FUNCTION_UNAVAILABLE(msg) __attribute__((unavailable(msg)))
#elif (__GNUC__ > 4) || (__GNUC__ >=4 && __GNUC_MINOR__ >= 4)
/* this might not warn if optimization removes a call, but it's better than nothing.
 * disable inlining to encourage GCC not to eliminate the call
 */
#define MONO_FUNCTION_UNAVAILABLE(msg) __attribute__((noinline,error(msg)))
#else
#define MONO_FUNCTION_UNAVAILABLE(msg) /* empty */
#endif

And then do this in this header (the implementation in helpers.c should be entirely within the #ifdef with no #else part):

#ifndef DISABLE_LOGGING
const char* mono_inst_name (int op);
#else
MONO_FUNCTION_UNAVAILABLE("mono_inst_name is not available when DISABLE_LOGGING is defined") const char *mono_inst_name (int op);
#endif

[lambdageek]

Do I have to add #ifndef DISABLE_LOGGING before all of the function call of mono_inst_name() to pass the tests?

Yes, you'll need to update very place that calls mono_inst_name. Some of them are already inside #ifdefs for logging (for example the entire src/mono/mono/mini/cfgdump.c file is in a giant ifdef)

[sayeedkhannabil]

I could not figure out why some test cases are failing. Besides src/mono/mono/mini/cfgdump.c I have enclosed with #ifndef DISABLE_LOGGING in every places where mono_inst_name is being used. I did not do so in cfdump.c because it was already checking if DISABLE_LOGGING is enabled in the start of the code. Is that the case why some test cases are failing?

[SamMonoRT]

@sayeedkhannabil - please resolve conflict. That will trigger a new CI run. We can help you investigate the failures once we have a newer CI check run.
cc @ivanpovazan

[ivanpovazan]

@sayeedkhannabil is there anyway I could help you move forward with this PR?

[sayeedkhannabil]

@sayeedkhannabil is there anyway I could help you move forward with this PR?

Hi. I am struggling with the CI/CD errors. Since I do not have any experience in that. If someone can help me figure out, how to work around with the CI/CD error, or give me some pointers how I can test this locally in my machine, that would be helpful for me. Thanks

[ivanpovazan]

@sayeedkhannabil is there anyway I could help you move forward with this PR?

Hi. I am struggling with the CI/CD errors. Since I do not have any experience in that. If someone can help me figure out, how to work around with the CI/CD error, or give me some pointers how I can test this locally in my machine, that would be helpful for me. Thanks

Sure, I will gladly help you out with that.

• The first thing that has to be done is to resolve conflicts that this PR has with the current tip of the main branch.
• Once you resolve the conflicts please push those changes. That will trigger CI to do another round of testing.
• After we get those results I can look into the failures to help you figure out whether your change introduced a regression or not.

[SamMonoRT]

@sayeedkhannabil - if the PR isn't ready, consider marking as Draft PR.

[sayeedkhannabil]

Sorry for the late reply and delay on the submission. I got overwhelmed by the large codebase and it took me a while to figure out how to run tests on my local machine. I have submitted a new pr, which passed all of the CI/CD. Please review it when you have the time. Again, sorry it took such a long time. Thanks, Abu Sayeed Khan

…

On Tue, Aug 22, 2023 at 9:40 AM Sam Patel ***@***.***> wrote: @sayeedkhannabil <https://github.com/sayeedkhannabil> - if the PR isn't ready, consider marking as Draft PR. — Reply to this email directly, view it on GitHub <#83570 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG576V5WUGW4VE5HEOJ3L63XWTAFTANCNFSM6AAAAAAV6DQZCE> . You are receiving this because you were mentioned.Message ID: ***@***.***>