Fix gcc warnings

[sfeifer]

Several gcc warnings were found in the libvalkey sources when building Performance Co-Pilot with libvalkey. The async.c and sds.c files have shadowed declaration warnings. In valkey.c and net.c, there are ignored return values. This PR fixes those warnings.

async.c:

deps/libvalkey/src/async.c: In function ‘valkeyGetSubscribeCallback’:
deps/libvalkey/src/async.c:523:32: warning: declaration of ‘cb’ shadows a previous local [-Wshadow]
  523 |                 valkeyCallback cb;
      |                                ^~
deps/libvalkey/src/async.c:466:21: note: shadowed declaration is here
  466 |     valkeyCallback *cb = NULL;
      |                     ^~
deps/libvalkey/src/async.c: In function ‘valkeySsubscribeCallback’:
deps/libvalkey/src/async.c:944:24: warning: declaration of ‘cb’ shadows a previous local [-Wshadow]
  944 |         valkeyCallback cb = {0};
      |                        ^~
deps/libvalkey/src/async.c:900:21: note: shadowed declaration is here
  900 |     valkeyCallback *cb = NULL;

sds.c:

deps/libvalkey/src/sds.c: In function ‘sdsnewlen’:
deps/libvalkey/src/sds.h:94:45: warning: declaration of ‘sh’ shadows a previous local [-Wshadow]
   94 | #define SDS_HDR_VAR(T, s) struct sdshdr##T *sh = (struct sdshdr##T *)((s) - (sizeof(struct sdshdr##T)));
      |                                             ^~
deps/libvalkey/src/sds.c:112:9: note: in expansion of macro ‘SDS_HDR_VAR’
  112 |         SDS_HDR_VAR(8, s);
      |         ^~~~~~~~~~~
deps/libvalkey/src/sds.c:87:11: note: shadowed declaration is here
   87 |     void *sh;
      |           ^~
deps/libvalkey/src/sds.h:94:45: warning: declaration of ‘sh’ shadows a previous local [-Wshadow]
   94 | #define SDS_HDR_VAR(T, s) struct sdshdr##T *sh = (struct sdshdr##T *)((s) - (sizeof(struct sdshdr##T)));
      |                                             ^~
deps/libvalkey/src/sds.c:119:9: note: in expansion of macro ‘SDS_HDR_VAR’
  119 |         SDS_HDR_VAR(16, s);
      |         ^~~~~~~~~~~
deps/libvalkey/src/sds.c:87:11: note: shadowed declaration is here
   87 |     void *sh;
      |           ^~
deps/libvalkey/src/sds.h:94:45: warning: declaration of ‘sh’ shadows a previous local [-Wshadow]
   94 | #define SDS_HDR_VAR(T, s) struct sdshdr##T *sh = (struct sdshdr##T *)((s) - (sizeof(struct sdshdr##T)));
      |                                             ^~
deps/libvalkey/src/sds.c:126:9: note: in expansion of macro ‘SDS_HDR_VAR’
  126 |         SDS_HDR_VAR(32, s);
      |         ^~~~~~~~~~~
deps/libvalkey/src/sds.c:87:11: note: shadowed declaration is here
   87 |     void *sh;
      |           ^~
deps/libvalkey/src/sds.h:94:45: warning: declaration of ‘sh’ shadows a previous local [-Wshadow]
   94 | #define SDS_HDR_VAR(T, s) struct sdshdr##T *sh = (struct sdshdr##T *)((s) - (sizeof(struct sdshdr##T)));
      |                                             ^~
deps/libvalkey/src/sds.c:133:9: note: in expansion of macro ‘SDS_HDR_VAR’
  133 |         SDS_HDR_VAR(64, s);
      |         ^~~~~~~~~~~
deps/libvalkey/src/sds.c:87:11: note: shadowed declaration is here
   87 |     void *sh;
      |           ^~

valkey.c:

deps/libvalkey/src/valkey.c: In function ‘valkeySetError’:
deps/libvalkey/src/valkey.c:708:9: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  708 |         strerror_r(errno, c->errstr, sizeof(c->errstr));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

net.c:

deps/libvalkey/src/net.c: In function ‘valkeySetErrorFromErrno’:
deps/libvalkey/src/net.c:110:5: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  110 |     strerror_r(errorno, (char *)(buf + len), sizeof(buf) - len);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To fix the shadowed declarations in async.c and sds.c, I just renamed the variables.

For the ignored returned values, I assigned the return of strerror_r to a char *, errmsg. There is then a check to make sure errmsg is the same as what is put into the buffer when strerror_r executes. If not, the buffer will be populated with the string pointed to by errmsg. My understanding of strerror_r is that it could return a pointer to a string rather than writing to the buffer. This new logic handles that case.

[sfeifer]

The CI failures look to be caused by libvalkey using a different version of the strerror_r function than PCP. PCP defines _GNU_SOURCE while libvalkey does not. This leads to libvalkey using the POSIX version that will always populate the buffer and return an int. The GNU version returns a char* and might not write to the buffer. I'm looking into how to work this out, but open to suggestions :).

[natoscott]

@sfeifer I think the simplest fix for ...

| warning: ignoring return value of ‘strerror_r’

... will be to "cast" the return value to (void) and continue right on ignoring it as before.

[sfeifer]

@sfeifer I think the simplest fix for ...

| warning: ignoring return value of ‘strerror_r’

... will be to "cast" the return value to (void) and continue right on ignoring it as before.

I tried that earlier and agree that it would work for the POSIX strerror_r, but it does not silence the warning for the GNU version. I think this is because the return of the GNU strerror_r function needs to be checked in case the buffer was not populated as the return will contain the error string.

Here's the error in valkey.c with the void cast:

deps/libvalkey/src/valkey.c:708:15: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  708 |         (void)strerror_r(errno, c->errstr, sizeof(c->errstr));
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[sfeifer]

I changed how libvalkey is compiled in pcp to use the -U_GNU_SOURCE flag, which results in the POSIX strerror_r function being used. With this change, the ignored return value warnings are no longer present without any changes to the libvalkey sources (i.e. no modifications needed to net.c or valkey.c).

I have now updated the PR to only fix the shadowed declaration warnings in async.c and sds.c.

[bjosv]

Nice, I don't see this with gcc-15 and our currently enabled warnings, so I wonder if we should add -Wshadow.
Is there any known drawbacks?

I have found some statements that -Wshadow was added before GCC 3, and Clang 2.9

[sfeifer]

I think the drawback of using -Wshadow is the additional noise. The warnings can help point out where a variable should be named something different to avoid confusion. There might be scenarios, though, where we want to keep the variable name as is leading to a warning that will be there every time libvalkey is compiled.

[bjosv]

Sounds good, lets merge this. Thanks!