The HAS_CLOCK_GETTIME check is half-broken / there is dead and broken code in the Linux HAL

[psychon]

Hi,

during configure time, CMake prints:

-- Looking for clock_gettime in rt
-- Looking for clock_gettime in rt - found

This comes from this code:

lib60870/lib60870-C/CMakeLists.txt

Line 25 in 7d58626

check_library_exists(rt clock_gettime "time.h" CONFIG_SYSTEM_HAS_CLOCK_GETTIME)

One might assume that the result of this check is used here:

lib60870/lib60870-C/src/hal/time/unix/time.c

Line 13 in 7d58626

#ifdef CONFIG_SYSTEM_HAS_CLOCK_GETTIME

Let's verify this assumption:

diff --git a/lib60870-C/src/hal/time/unix/time.c b/lib60870-C/src/hal/time/unix/time.c
index 7cc6b06..96c7c88 100644
--- a/lib60870-C/src/hal/time/unix/time.c
+++ b/lib60870-C/src/hal/time/unix/time.c
@@ -11,6 +11,7 @@
 #include <time.h>
 
 #ifdef CONFIG_SYSTEM_HAS_CLOCK_GETTIME
+#error The check works
 uint64_t
 Hal_getTimeInMs()
 {
@@ -21,6 +22,7 @@ Hal_getTimeInMs()
        return ((uint64_t) tp.tv_sec) * 1000LL + (tp.tv_nsec / 1000000);
 }
 #else
+#error THE CHECK DOES NOT WORK
 
 #include <sys/time.h>
 

/tmp/lib60870/lib60870-C/src/hal/time/unix/time.c:25:2: error: #error THE CHECK DOES NOT WORK

The result of the check newer reaches the compiler. If we fix that:

diff --git a/lib60870-C/CMakeLists.txt b/lib60870-C/CMakeLists.txt
index 08cc987..270f672 100644
--- a/lib60870-C/CMakeLists.txt
+++ b/lib60870-C/CMakeLists.txt
@@ -23,6 +23,9 @@ endmacro()
 # feature checks
 include(CheckLibraryExists)
 check_library_exists(rt clock_gettime "time.h" CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
+if(CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
+    add_compile_definitions(CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
+endif()
 
 # check if we are on a little or a big endian
 include (TestBigEndian)

...then the result does not link. For example:

/usr/lib/ccache/cc -Wredundant-decls -rdynamic examples/cs104_server_files/CMakeFiles/cs104_server_files.dir/cs104_server_files.c.o -o examples/cs104_server_files/cs104_server_files  src/liblib60870.a  -lpthread  -lm  -lrt && :
/usr/bin/ld: src/liblib60870.a(file_server.c.o): in function `sendSegment':
file_server.c:(.text+0x6fc): undefined reference to `Hal_getMonotonicTimeInMs'
/usr/bin/ld: src/liblib60870.a(file_server.c.o): in function `CS101_FileServer_handleAsdu':
file_server.c:(.text+0x8c4): undefined reference to `Hal_getMonotonicTimeInMs'
/usr/bin/ld: file_server.c:(.text+0xa7d): undefined reference to `Hal_getMonotonicTimeInMs'
/usr/bin/ld: file_server.c:(.text+0xc69): undefined reference to `Hal_getMonotonicTimeInMs'
/usr/bin/ld: file_server.c:(.text+0xd31): undefined reference to `Hal_getMonotonicTimeInMs'
/usr/bin/ld: src/liblib60870.a(file_server.c.o):file_server.c:(.text+0xdb5): more undefined references to `Hal_getMonotonicTimeInMs' follow
collect2: error: ld returned 1 exit status

This is because the #if case only defines Hal_getTimeInMs and functions like ...InNs or ...Monotonic... only exist in the #else. And to my confusion, these function use clock_gettime() even when the check said that this function does not exist.

Thus, my conclusion is that clock_gettime() is an unconditional dependency and all the checks done around it are dead code. I propose something like the following change (I did not fix code indentation to make the diff simpler to read):

diff --git a/lib60870-C/CMakeLists.txt b/lib60870-C/CMakeLists.txt
index 08cc987..8626cde 100644
--- a/lib60870-C/CMakeLists.txt
+++ b/lib60870-C/CMakeLists.txt
@@ -20,10 +20,6 @@ macro(ADD_C_FLAGS flags)
     set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${flags}")
 endmacro()
 
-# feature checks
-include(CheckLibraryExists)
-check_library_exists(rt clock_gettime "time.h" CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
-
 # check if we are on a little or a big endian
 include (TestBigEndian)
 test_big_endian(PLATFORM_IS_BIGENDIAN)
diff --git a/lib60870-C/src/CMakeLists.txt b/lib60870-C/src/CMakeLists.txt
index 7773018..71b7397 100644
--- a/lib60870-C/src/CMakeLists.txt
+++ b/lib60870-C/src/CMakeLists.txt
@@ -149,18 +149,11 @@ GENERATE_EXPORT_HEADER(lib60870-shared
 add_library (lib60870 STATIC ${library_SRCS})
 
 IF(UNIX)
-  IF (CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
      target_link_libraries (lib60870
          -lpthread
          -lm
          -lrt
      )
-  ELSE ()
-     target_link_libraries (lib60870
-         -lpthread
-         -lm
-     )
-  ENDIF (CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
 
   configure_file(
     ${CMAKE_CURRENT_LIST_DIR}/lib60870.pc.in
diff --git a/lib60870-C/src/hal/CMakeLists.txt b/lib60870-C/src/hal/CMakeLists.txt
index 53bafdc..04e2a9a 100644
--- a/lib60870-C/src/hal/CMakeLists.txt
+++ b/lib60870-C/src/hal/CMakeLists.txt
@@ -13,10 +13,6 @@ set(LIBHAL_VERSION_MAJOR "2")
 set(LIBHAL_VERSION_MINOR "2")
 set(LIBHAL_VERSION_PATCH "0")
 
-# feature checks
-include(CheckLibraryExists)
-check_library_exists(rt clock_gettime "time.h" CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
-
 # check if we are on a little or a big endian
 include (TestBigEndian)
 test_big_endian(PLATFORM_IS_BIGENDIAN)
@@ -189,16 +185,10 @@ SET_TARGET_PROPERTIES(hal-shared PROPERTIES
 )
 
 IF(UNIX)
-  IF (CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
      target_link_libraries (hal
          -lpthread
          -lrt
      )
-  ELSE ()
-     target_link_libraries (hal
-         -lpthread
-     )
-  ENDIF (CONFIG_SYSTEM_HAS_CLOCK_GETTIME)
 ENDIF(UNIX)
 
 IF(CONFIG_USE_EXTERNAL_MBEDTLS_DYNLIB)
diff --git a/lib60870-C/src/hal/time/unix/time.c b/lib60870-C/src/hal/time/unix/time.c
index 7cc6b06..eefad1f 100644
--- a/lib60870-C/src/hal/time/unix/time.c
+++ b/lib60870-C/src/hal/time/unix/time.c
@@ -9,19 +9,6 @@
 
 #include "hal_time.h"
 #include <time.h>
-
-#ifdef CONFIG_SYSTEM_HAS_CLOCK_GETTIME
-uint64_t
-Hal_getTimeInMs()
-{
-	struct timespec tp;
-
-	clock_gettime(CLOCK_REALTIME, &tp);
-
-	return ((uint64_t) tp.tv_sec) * 1000LL + (tp.tv_nsec / 1000000);
-}
-#else
-
 #include <sys/time.h>
 
 msSinceEpoch
@@ -92,5 +79,3 @@ Hal_getMonotonicTimeInNs()
 
     return nsTime;
 }
-
-#endif

Feel free to disagree and do something else about this. This ticket just exists to inform you about this dead code.

(Edit: And I am unsure if stopping to use clock_gettime and using gettimeofday instead is the intended approach. I did it like that in the above diff because that is what the existing code was already doing before.)

(Some of the functions in the HAL also confuse me. Like, why does HAL_setTimeInNs() exist? Why would the library ever want to set the system clock? This function also has no callers and just makes this library larger for no good reason, IMHO.)

[mzillgith]

Hi,
Thank you for comments.
I agree with your proposal. The code using gettimeofday was not maintained and was to support some older versions of Linux.
I cleaned it up as you proposed.