[Piglit] [PATCH 03/10] piglit-util: add piglit_get_microseconds

[Ian Romanick]

On 10/16/2013 03:45 PM, Chad Versace wrote:
> On 10/15/2013 04:33 PM, Jordan Justen wrote:
>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Chad Versace <chad.versace at linux.intel.com>
>> ---
>> Chad, this conflicts with your "Detect if system has POSIX clocks" patch,
>> but it could be tweaked to follow your version.
> 
> I'm not opposed to defining a piglit_get_microseconds(). However, I'd
> rather layer it on top of my already submitted POSIX clocks patch.

Why is one approach for detecting clock_gettime better or worse than the
other?  I assume it's more than just whatever you saw when you opened
the coffin lid on check_function_exists...

> Issues below...
> 
>>
>>   CMakeLists.txt           |    6 ++++++
>>   tests/util/piglit-util.c |   17 +++++++++++++++++
>>   tests/util/piglit-util.h |    8 ++++++++
>>   3 files changed, 31 insertions(+)
>>
>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>> index 4ec5ddf..c876a05 100644
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -6,6 +6,7 @@ INCLUDE (CheckCCompilerFlag)
>>   INCLUDE (CheckCXXCompilerFlag)
>>   INCLUDE (CheckFunctionExists)
>>   INCLUDE (CheckIncludeFile)
>> +INCLUDE (CheckLibraryExists)
>>   INCLUDE (FindPkgConfig)
>>
>>   project (piglit)
>> @@ -294,6 +295,11 @@ check_function_exists(strchrnul HAVE_STRCHRNUL)
>>   check_function_exists(strndup   HAVE_STRNDUP)
>>   check_function_exists(fopen_s   HAVE_FOPEN_S)
>>   check_function_exists(setrlimit HAVE_SETRLIMIT)
>> +check_function_exists(clock_gettime HAVE_CLOCK_GETTIME_FUNC)
> 
> I just inspected CMake's implementation of check_function_exists(),
> and now I am strongly against using it anywhere. If you saw its
> implementation,
> you would likely agree too. You find it in
> /usr/share/cmake/Modules/CheckFunctionExists.{c,cmake}.

Well... they try to support pre-ANSI C, so that's a strike against them.
 It's fairly ugly, but, off the top of my head, I can't think of a
better way that is still generic.

>> +check_library_exists(rt clock_gettime "time.h" HAVE_CLOCK_GETTIME_RT)
>> +if(HAVE_CLOCK_GETTIME_FUNC OR HAVE_CLOCK_GETTIME_RT)
>> +    add_definitions(-DHAVE_CLOCK_GETTIME)
>> +endif()
>>
>>   check_include_file(sys/time.h  HAVE_SYS_TIME_H)
>>   check_include_file(sys/types.h HAVE_SYS_TYPES_H)
>> diff --git a/tests/util/piglit-util.c b/tests/util/piglit-util.c
>> index 71d55a7..18df0b6 100644
>> --- a/tests/util/piglit-util.c
>> +++ b/tests/util/piglit-util.c
>> @@ -33,6 +33,10 @@
>>   #include <string.h>
>>   #include <errno.h>
>>
>> +#ifdef HAVE_CLOCK_GETTIME
>> +#include <time.h>
>> +#endif
>> +
>>   #include "config.h"
>>   #if defined(HAVE_SYS_TIME_H) && defined(HAVE_SYS_RESOURCE_H) &&
>> defined(HAVE_SETRLIMIT)
>>   #include <sys/time.h>
>> @@ -462,3 +466,16 @@ write_null:
>>       va_end(va);
>>       return size_written;
>>   }
>> +
>> +uint64_t
>> +piglit_get_microseconds(void)
>> +{
>> +#ifdef HAVE_CLOCK_GETTIME
>> +    struct timespec t;
>> +    clock_gettime(CLOCK_MONOTONIC, &t);
>> +    return (t.tv_sec * 1000000) + (t.tv_nsec / 1000);
>> +#else
>> +    return (uint64_t) 0;
>> +#endif
>> +}
> 
> I'm opposed to implementing broken functions. If this function is not yet
> implemented for some system, then tests that require it should either skip
> or not even build. This patch's approach, though, causes such tests to
> fail.

I think having piglit_get_microseconds return int64_t, where -1 is
error, will fix the problem.  Then tests can do:

    if ((t = piglit_get_microseconds()) < 0) {
        // skip
    }

This also lets us handle the case where clock_gettime returns an error.

>> +
>> diff --git a/tests/util/piglit-util.h b/tests/util/piglit-util.h
>> index 52f053e..6f64ccf 100644
>> --- a/tests/util/piglit-util.h
>> +++ b/tests/util/piglit-util.h
>> @@ -174,6 +174,14 @@ piglit_source_dir(void);
>>   size_t
>>   piglit_join_paths(char buf[], size_t buf_size, int n, ...);
>>
>> +/**
>> + * \brief Get a monotonically increasing time in microseconds
>> + *
>> + * This time can be used for relative time measurements.
>> + */
>> +uint64_t
>> +piglit_get_microseconds(void);
>> +
>>   #ifdef __cplusplus
>>   } /* end extern "C" */
>>   #endif