fix compile warning of shadowing a previous local [-Wshadow]

[wkliao]

This is to fix the first part of #1077 (compilation warning on shadowed declaration).

Note this PR stacks on top of #1082, because file darshan-apmpi.c
from autoperf submodule also needs to be fixed.

This PR needs to be rebased on #1084 to pass CI end_to_end_regression.yml

[carns]

Would it be better to put the static int __darshan_disabled; statements in darshan.h so that it doesn't have to be explicitly declared in all of the .c files?

[wkliao]

I have made changes accordingly.

[wkliao]

Well, the side effect of this is a compilation warning on defined but not used variable of __darshan_disabled for the following files.

../../../darshan/darshan-runtime/lib/darshan-config.c:16:
../../../darshan/darshan-runtime/lib/darshan-common.c:22:
../../../darshan/darshan-runtime/lib/darshan-ldms.c:17:
../../../darshan/darshan-runtime/lib/darshan-dxt.c:34:
../../../darshan/darshan-runtime/lib/darshan-heatmap.c:23:
../../../darshan/darshan-runtime/lib/darshan-config.c:16:
../../../darshan/darshan-runtime/lib/darshan-ldms.c:17:

[wkliao]

I revert the changes, as not all files including darshan.h make use of
variable __darshan_disabled.

[carns]

Something feels weird to me about this. It is odd for some of the .c files to have to declare __darshan_disabled as a visible variable even if they aren't explicitly using the variable otherwise.

Superficially, we could make __darshan_disabled a global defined in darshan-core.c, and then put an extern reference to it in darshan.h. For that matter, if we did that, then we could also eliminate the darshan_core_disabled_instrumentation() function entirely, and anyone who needs access to that flag (including the special case in the rename() wrapper) could just use it directly. The value could be toggled explicitly in darshan-core.c during shutdown and init once the __darshan_core pointer is ready for use.

Looking deeper, I'm trying to figure out what the variable is accomplishing in general. It's value is determined by checking if __darshan_core is set while a lock is held, but then the variable itself is used without a lock. I think this indirection is meant to avoid excessive lock/unlock cost in all of the macros that check it, but it feels like an awkward way to accomplish that. I guess that is a longer term question beyond the scope of this PR, though.

What do you think? Should we just do the eliminate the darshan_core_disabled_instrumentation() function and make this an extern that anyone who includes darshan.h can access?

[carns]

Something feels weird to me about this. It is odd for some of the .c files to have to declare __darshan_disabled as a visible variable even if they aren't explicitly using the variable otherwise.

Superficially, we could make __darshan_disabled a global defined in darshan-core.c, and then put an extern reference to it in darshan.h. For that matter, if we did that, then we could also eliminate the darshan_core_disabled_instrumentation() function entirely, and anyone who needs access to that flag (including the special case in the rename() wrapper) could just use it directly. The value could be toggled explicitly in darshan-core.c during shutdown and init once the __darshan_core pointer is ready for use.

Looking deeper, I'm trying to figure out what the variable is accomplishing in general. It's value is determined by checking if __darshan_core is set while a lock is held, but then the variable itself is used without a lock. I think this indirection is meant to avoid excessive lock/unlock cost in all of the macros that check it, but it feels like an awkward way to accomplish that. I guess that is a longer term question beyond the scope of this PR, though.

What do you think? Should we just do the eliminate the darshan_core_disabled_instrumentation() function and make this an extern that anyone who includes darshan.h can access?

I feel like I'm missing something about why this is set up this way 🫤

[wkliao]

I do not know much about darshan_core_disabled_instrumentation.
But, my reading into the codes is that __darshan_disabled appears to be used
locally within each calling function. The right fix is to make it a local variable.

I do not like my solution in this PR which makes it a locally global variable per file.
If can, I would avoid any global variable as mush as possible.
This PR is just a quick fix, with a minimal change to the codes.

[wkliao]

The latest commit, ac1809b, makes __darshan_disabled a local variable

[carns]

Ok, I see what you mean. This iteration is the biggest change since it requires introducing a variable declaration in so many places, but I think you are right that it is the safest option. That way the value won't change in the middle of a wrapper.