clang20 cleanups

[roblatham00]

When I build with clang-20 I get a few warnings. The "uninitialized variable" and 'don't use K&R syntax' changes are straightforward enough.

I'm really puzzled by the newpath != path part -- that's one of those "how did it ever work in the first place?" things, I'd like a second set of eyes because there's got to be a reason we were doing it that way since January 2019 (db25337)

[wkliao]

I had a quick look at line 231 of file darshan-runtime/lib/darshan-stdio.c

225          __newpath = darshan_clean_file_path(__path); \

231 -      if(__newpath != (char*)__path) free(__newpath); \
231 +      if(strcmp(__newpath, (char*)__path) != 0 ) free(__newpath); \

My reading to it is if __newpath is newly allocated string by a call
to darshan_clean_file_path() at line 225, then free its allocated space.

[roblatham00]

ok so we need to take a look at darshan_clean_file_path, which takes a string and returns one of a few options:

• NULL if the string is a special path like <STDIN>
• an allocated copy of that string if it is an absolute path,
• an allocated construction of an absolute path if string is a relative path

in the 2nd and 3rd cases the routine also takes out extra '/' and '/./` in the string

226 if(!__newpath) __newpath = (char*)__path;

so we are dealing with cases where darshan_clean_file_path allocated memory and cleaning up.

Clang is fine with most of this except when the path in question is a string literal:

  CC       libdarshan_la-darshan-stdio.lo
../../../darshan-runtime/lib/darshan-stdio.c:1094:5: warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare]
 1094 |     STDIO_RECORD_OPEN(stdin, "<STDIN>", 0, 0);
      |     ^                        ~~~~~~~~~
../../../darshan-runtime/lib/darshan-stdio.c:231:22: note: expanded from macro 'STDIO_RECORD_OPEN'
  231 |         if(__newpath != __path) free(__newpath); \
      |                      ^  ~~~~~~

so... strcmp is probably not the right thing here... we don't care if the strings are identical we are trying to figure out if darshan_clean_file_path allocated anything

We can't leave newpath null: e.g. stdio_track_new_file_record won't know that NULL means ''

We could forbid passing string literals to RECORD_OPEN.... makes for more work for the callers but string literals are only passed in during initalize

We could alloca a buffer in the macro , pass that to darshan_clean_file_path, and then never worry about freeing the path. A little warry about messing with the stack, though.

darshan_clean_file_path could set a "needs_cleanup" flag... gets the job done but gross

[roblatham00]

ok well new approach: just keep doing what we are doing but I just pushed a new version that puts a big comment explaining away the warning

[wkliao]

I took a look at subroutine darshan_clean_file_path() and found that it is already
doing the malloc. It returns either NULL or a newly allocated space. When NULL,
line 226 assigns __newpath to __path.

226        if(!__newpath) __newpath = (char*)__path; \

Maybe try the following to see if Clang still complains.

231       if((void*)__newpath != (void*)__path) free(__newpath); \

[carns]

Yeah I like the idea of big comment (what you have now is great) + type cast to convince clang that we know what we are up to here :)

[roblatham00]

casting to void didn't fool clang.

some rando on reddit suggests "try to write to the string and catch the segv" -- uh no thank you i'll keep the warning!

[roblatham00]

I don't know if this solution is better than the problem... just don't pass string literals to that macro...

[carns]

That's not so bad. @wkliao what do you think?

[wkliao]

How about using strdup()?

--- a/darshan-runtime/lib/darshan-stdio.c
+++ b/darshan-runtime/lib/darshan-stdio.c
@@ -223,16 +223,16 @@ extern int __real_fileno(FILE *stream);
     (void)__darshan_disabled; \
     if(!__ret || !__path) break; \
     __newpath = darshan_clean_file_path(__path); \
-    if(!__newpath) __newpath = (char*)__path; \
+    if(!__newpath) __newpath = strdup(__path); \
     __rec_id = darshan_core_gen_record_id(__newpath); \
     __rec_ref = darshan_lookup_record_ref(stdio_runtime->rec_id_hash, &__rec_id, sizeof(darshan_record_id)); \
     if(!__rec_ref) __rec_ref = stdio_track_new_file_record(__rec_id, __newpath); \
     if(!__rec_ref) { \
-        if(__newpath != (char*)__path) free(__newpath); \
+        free(__newpath); \
         break; \
     } \
     _STDIO_RECORD_OPEN(__ret, __rec_ref, __tm1, __tm2, 1, -1); \
-    if(__newpath != (char*)__path) free(__newpath); \
+    free(__newpath); \
     /* LDMS to publish realtime open tracing information to daemon*/ \
     if(dC.ldms_lib)\
         if(dC.stdio_enable_ldms)\

[roblatham00]

Thanks wei-keng I like that approach a lot. Pushed a new revision that does just that. no compiler warnings and cleaner code. win win.

[wkliao]

One of github action jobs failed with an error related to the installation of h5py-3.14.0.
The same error happened to other recent PRs as well.

[carns]

Yeah, the h5py problem is unrelated to this PR, I'll open up a separate issue about that.