POETSII / tinsel

Manythread RISC-V overlay for FPGA clusters
Other
35 stars 1 forks source link

Explicitly initialise 'buf' in 'alive' and 'socketAlive'. #113

Closed mvousden closed 2 years ago

mvousden commented 2 years ago

While making the fix in https://github.com/POETSII/tinsel/pull/112, I found a couple more warnings while building hostlink:

g++ -Wall -I ../hostlink -O2 pciestreamd.cpp -o pciestreamd
pciestreamd.cpp: In function ‘int alive(int)’:
pciestreamd.cpp:81:14: warning: ‘buf’ may be used uninitialized [-Wmaybe-uninitialized]
   81 |   return send(sock, &buf, 0, 0) == 0;
      |          ~~~~^~~~~~~~~~~~~~~~~~
In file included from pciestreamd.cpp:15:
/usr/include/sys/socket.h:138:16: note: by argument 2 of type ‘const void*’ to ‘ssize_t send(int, const void*, size_t, int)’ declared here
  138 | extern ssize_t send (int __fd, const void *__buf, size_t __n, int __flags);
      |                ^~~~
pciestreamd.cpp:80:8: note: ‘buf’ declared here
   80 |   char buf;
      |        ^~~
[...]
g++ -I ../hostlink -o SocketUtils.o -I/home/mark/repos/tinsel/include -O2 -Wall -c SocketUtils.cpp
SocketUtils.cpp: In function ‘bool socketAlive(int)’:
SocketUtils.cpp:26:14: warning: ‘buf’ may be used uninitialized [-Wmaybe-uninitialized]
   26 |   return send(conn, &buf, 0, 0) == 0;
      |          ~~~~^~~~~~~~~~~~~~~~~~
In file included from SocketUtils.cpp:9:
/usr/include/sys/socket.h:138:16: note: by argument 2 of type ‘const void*’ to ‘ssize_t send(int, const void*, size_t, int)’ declared here
  138 | extern ssize_t send (int __fd, const void *__buf, size_t __n, int __flags);
      |                ^~~~
SocketUtils.cpp:25:8: note: ‘buf’ declared here
   25 |   char buf;
      |        ^~~
[...]

This changeset removes these warnings by initialising buf to zero (which I suspect it is anyway).

I suspect this will also help with Valgrind debugging in the Orchestrator, because we often receive warnings about uninitialised buf from the latter of these two functions. Not tested that though.

I've compiled this, but I haven't run it on anything (I don't believe I have the means to do so), so this needs some subjecting to your CI before merging I think.

coralmw commented 2 years ago

Passes the simulation smoke test (apps/hello), LGTM.