Any chance to modernize the code?

[juj]

It looks like the benchmarks do no longer build with modern C compilers due to use of very old C dialect. Any chance of updating the benchmarks to modern syntax?

[juj]

For illustration, here are the set of changes I did to make the code pass on latest LLVM/Clang 16. Would these look like they'd be adoptable?

diff --git a/bench/CMakeLists.txt b/bench/CMakeLists.txt
index 3ed27ac..4be2b76 100644
--- a/bench/CMakeLists.txt
+++ b/bench/CMakeLists.txt
@@ -38,9 +38,11 @@ set(barnes_sources
 PREPEND(barnes_sources barnes/ ${barnes_sources})
 
 # turn off warnings..
+message(STATUS "${CMAKE_C_COMPILER_ID}")
 if(CMAKE_C_COMPILER_ID MATCHES "AppleClang|Clang|GNU")
-  list(APPEND CMAKE_C_FLAGS " -w ")
-  list(APPEND CMAKE_CXX_FLAGS " -w ")
+  set(FLAGS " -w -Wno-implicit-function-declaration -Wno-implicit-int -Wno-int-conversion")
+  list(APPEND CMAKE_C_FLAGS ${FLAGS})
+  list(APPEND CMAKE_CXX_FLAGS ${FLAGS})
 endif()
 
 add_executable(cfrac ${cfrac_sources})
@@ -69,16 +71,6 @@ add_executable(alloc-test alloc-test/test_common.cpp alloc-test/allocator_tester
 target_compile_definitions(alloc-test PRIVATE BENCH=4)
 target_link_libraries(alloc-test pthread)
 
-if(NOT APPLE)
-  add_executable(sh6bench shbench/sh6bench-new.c)
-  target_compile_definitions(sh6bench PRIVATE BENCH=1 SYS_MULTI_THREAD=1)
-  target_link_libraries(sh6bench pthread)
-
-  add_executable(sh8bench shbench/sh8bench-new.c)
-  target_compile_definitions(sh8bench PRIVATE BENCH=1 SYS_MULTI_THREAD=1)
-  target_link_libraries(sh8bench pthread)
-endif()
-
 add_executable(cache-scratch cache-scratch/cache-scratch.cpp)
 target_link_libraries(cache-scratch pthread)
 
diff --git a/bench/barnes/code.c b/bench/barnes/code.c
index bb7142e..8295a85 100644
--- a/bench/barnes/code.c
+++ b/bench/barnes/code.c
@@ -68,7 +68,8 @@ Command line options:
        Default is 1.
 */
 
- 
+void pranset(int seed);
+void error(const char *format, ...);
 
 #define global  /* nada */
 
@@ -143,11 +144,11 @@ string argv[];
    }
 
    /* Make the master do slave work so we don't waste the processor */
-   {long time(); (Global->computestart) = time(0);};
+   { (Global->computestart) = time(0);};
    printf("COMPUTESTART  = %12u\n",Global->computestart);
    SlaveStart();
 
-   {long time(); (Global->computeend) = time(0);};
+   { (Global->computeend) = time(0);};
 
    {;};
 
@@ -502,7 +503,7 @@ stepsystem (ProcessId)
     bodyptr p,*pp;
     vector acc1, dacc, dvel, vel1, dpos;
     int intpow();
-    unsigned int time;
+    unsigned int time_;
     unsigned int trackstart, trackend;
     unsigned int partitionstart, partitionend;
     unsigned int treebuildstart, treebuildend;
@@ -514,7 +515,7 @@ stepsystem (ProcessId)
     }
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (trackstart) = time(0);};
+        { (trackstart) = time(0);};
     }
 
     if (ProcessId == 0) {
@@ -530,14 +531,14 @@ stepsystem (ProcessId)
     {;};
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (treebuildstart) = time(0);};
+        { (treebuildstart) = time(0);};
     }
 
     /* load bodies into tree   */
     maketree(ProcessId);
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (treebuildend) = time(0);};
+        { (treebuildend) = time(0);};
         Global->treebuildtime += treebuildend - treebuildstart;
     }
 
@@ -549,7 +550,7 @@ stepsystem (ProcessId)
 				      + (ProcessId == (NPROC - 1)));
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (partitionstart) = time(0);};
+        { (partitionstart) = time(0);};
     }
 
     Local[ProcessId].mynbody = 0;
@@ -557,18 +558,18 @@ stepsystem (ProcessId)
 
 /*     B*RRIER(Global->Barcom,NPROC); */
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (partitionend) = time(0);};
+        { (partitionend) = time(0);};
         Global->partitiontime += partitionend - partitionstart;
     }
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (forcecalcstart) = time(0);};
+        { (forcecalcstart) = time(0);};
     }
 
     ComputeForces(ProcessId);
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (forcecalcend) = time(0);};
+        { (forcecalcend) = time(0);};
         Global->forcecalctime += forcecalcend - forcecalcstart;
     }
 
@@ -608,7 +609,7 @@ stepsystem (ProcessId)
     {;};
 
     if ((ProcessId == 0) && (Local[ProcessId].nstep >= 2)) {
-        {long time(); (trackend) = time(0);};
+        { (trackend) = time(0);};
         Global->tracktime += trackend - trackstart;
     }
     if (ProcessId==0) {
@@ -704,7 +705,7 @@ find_my_bodies(mycell, work, direction, ProcessId)
       for (i = 0; i < l->num_bodies; i++) {
 	 if (work >= Local[ProcessId].workMin - .1) {
 	    if((Local[ProcessId].mynbody+2) > maxmybody) {
-	       error("find_my_bodies: Processor %d needs more than %d bodies; increase fleaves\n",ProcessId, maxmybody); 
+	       error("find_my_bodies: Processor %d needs more than %d bodies; increase fleaves\n",ProcessId, maxmybody);
 	    }
 	    Local[ProcessId].mybodytab[Local[ProcessId].mynbody++] = 
 	       Bodyp(l)[i];
diff --git a/bench/barnes/code_io.c b/bench/barnes/code_io.c
index 01d5948..d7dac89 100644
--- a/bench/barnes/code_io.c
+++ b/bench/barnes/code_io.c
@@ -28,6 +28,7 @@
 void in_int (), in_real (), in_vector ();
 void out_int (), out_real (), out_vector ();
 void diagnostics (unsigned int ProcessId);
+void error(const char *format, ...);
     
 /*
  * INPUTDATA: read initial conditions from input file.
diff --git a/bench/barnes/getparam.c b/bench/barnes/getparam.c
index fd636cf..91b6f54 100644
--- a/bench/barnes/getparam.c
+++ b/bench/barnes/getparam.c
@@ -24,6 +24,8 @@
 
 #include "stdinc.h"
 
+void error(const char *format, ...);
+
 local string *defaults = NULL;        /* vector of "name=value" strings */
 
 /*
diff --git a/bench/barnes/load.c b/bench/barnes/load.c
index ecd4054..7070343 100644
--- a/bench/barnes/load.c
+++ b/bench/barnes/load.c
@@ -23,6 +23,8 @@
 #include "code.h"
 #include "defs.h"
 
+void error(const char *format, ...);
+
 bool intcoord();
 cellptr makecell(unsigned int ProcessId);
 leafptr makeleaf(unsigned int ProcessId);
diff --git a/bench/barnes/util.c b/bench/barnes/util.c
index 34fd195..3aa4346 100644
--- a/bench/barnes/util.c
+++ b/bench/barnes/util.c
@@ -19,6 +19,7 @@
 
 #include <stdio.h>
 #include <errno.h>
+#include <stdarg.h>
 #include "stdinc.h"
 
 #define HZ 60.0
@@ -76,6 +77,7 @@ prand()
 #include <sys/types.h>
 #include <sys/times.h>
 
+void error(const char *format, ...);
 
 double cputime()
 {
@@ -87,17 +89,19 @@ double cputime()
 }
 
 /*
- * ERROR: scream and die quickly.
+ * ERROR: output a printf-formatted error to stderr, print
+ * errno error if present, and abort program execution.
  */
 
-error(msg, a1, a2, a3, a4)
-  char *msg, *a1, *a2, *a3, *a4;
+void error(const char *format, ...)
 {
-   extern int errno;
-
-   fprintf(stderr, msg, a1, a2, a3, a4);
+   va_list args;
+   va_start(args, format);
+   char str[256];
+   vsnprintf(str, 256, format, args);
+   va_end(args);
+   fprintf(stderr, "%s\n", str);
    if (errno != 0)
       perror("Error");
    exit(0);
 }
-

[daanx]

Ah very nice! I am ok with updating benchmarks to make them compile but ..

• each benchmark should have its own PR (as they all have different licenses and terms); We may also add a note to the README of the benchmark depending on the extent of the changes)
• and we should do minimal changes -- only to make it compile but be careful not to change behavior (At least with the older more standard ones that occur in earlier paper). I think you already do this in the example above -- except for the fprintf I guess?

Does this make sense? Maybe @jvoisin or @mjp41 have an opinion too about this?
Thanks,
-- Daan

[jvoisin]

I'm all for it, but indeed meaningful PR by PR, and upstream as much as possible.

[mjp41]

I agree with the approach @daanx sets out.