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

Chad Versace chad.versace at linux.intel.com
Thu Oct 17 02:08:34 CEST 2013


On 10/16/2013 04:03 PM, Ian Romanick wrote:
> 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.

Do you see anything wrong with this? Not only does it check that the
symbol is present, but it checks that all the pieces are there including
the headers and symbols we want.

check_c_source_compiles(
         HAVE_CLOCK_GETTIME
         "
         #define POSIX_C_SOURCE >= 199309L
         #include <time.h>
         int main() { return clock_gettime(CLOCK_MONOTONIC, NULL); }
         "
)

If a library is required for linking, then you can pass that in too.

>>> +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.

That's a sensible approach.


More information about the Piglit mailing list