Fixes from updating igt packaging in Fedora

From: Lyude Paul <[email protected]>

For work reasons, I've been tasked with getting Fedora running portions of IGT in our own CI infrastructure. In doing so I needed to get our RPM packaging up to date, building and passing the meson unit tests on Fedora 29 and Fedora Rawhide. This ended up exposing some rather interesting bugs, and also prompted me to update the Dockerfile and Gitlab CI settings that we use for testing builds. So, here's my fixes and improvements!

Since a couple of people have been asking if I have a gitlab branch that they can check to make sure that the CI changes work, here it is:

https://gitlab.freedesktop.org/lyudess/igt-gpu-tools/tree/wip/fedora-docker-update-v3

Changes since v1
* Add one last patch that came out of package review - soversioning, which both Debian and Fedora seem to require
(see https://bugzilla.redhat.com/show_bug.cgi?id=1700559 )

Changes since v2
* Don't get rid of assert() apparently, instead perma-ban -Db_ndebug!=false
* Fix usages of the gettid() syscall that weren't wrapped around the gettid() macro
* Also typecast gettid() correctly to pid_t and update users, to avoid introducing warnings when using new glibc's version of gettid() and for general correctness

Lyude Paul (9):
lib/tests: Fix test failures with meson 0.50.0
Use gettid() wrapper everywhere
lib/aux: Typecast gettid() as pid_t
meson: Don't redefine gettid if the C library provides it
meson: Add .so versioning
meson: Don't allow building with NDEBUG, ever lib/igt_core: Just use igt_can_fail() in __igt_run_subtest() Use pkgconfig() macros with dnf in Fedora Dockerfile Update Dockerfile.fedora to Fedora 29

 Dockerfile.fedora           | 50 ++++++++++++++++++-------------------
 benchmarks/gem_syslatency.c |  1 -
 lib/igt_aux.h               | 11 +++++++-
 lib/igt_core.c              |  6 ++---
 lib/igt_core.h              |  2 +-
 lib/meson.build             |  3 ++-
 meson.build                 | 13 ++++++++++
 tests/core_auth.c           |  6 ++---
 tests/drm_import_export.c   |  2 +-
 tests/i915/gem_close_race.c |  1 -
 tools/meson.build           |  3 ++-
 11 files changed, 57 insertions(+), 41 deletions(-)

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

Since meson 0.50.0, unit tests which return the GNU standard return code 99 will fail, regardless of whether or not should_fail:true is passed to test(). Unfortunately, our standard error code (IGT_EXIT_FAILURE) is also 99.

So, fix this by changing our standard error code to 98.

---
 lib/igt_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 47ffd9e7..44634e0f 100644
@@ -119,7 +119,7 @@ extern char *igt_frame_dump_path;
  *
  * Exit status indicating a test failure
  */
-#define IGT_EXIT_FAILURE 99
+#define IGT_EXIT_FAILURE 98
 
 bool __igt_fixture(void);
 void __igt_fixture_complete(void);

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

Currently we have multiple different parts of IGT that define their own wrapper around the gettid() syscall (or just call it directly with no wrapper).

Additionally, add the appropriate #includes for igt_aux.h to make sure syscall() is available.

---
 benchmarks/gem_syslatency.c | 1 -
 lib/igt_aux.h               | 7 ++++++-
 lib/igt_core.c              | 2 +-
 tests/core_auth.c           | 6 ++----
 tests/drm_import_export.c   | 2 +-
 tests/i915/gem_close_race.c | 1 -
 6 files changed, 10 insertions(+), 9 deletions(-)

index ba5d32a9..7671dc43 100644
@@ -44,7 +44,6 @@
 
 #include <linux/unistd.h>
 
-#define gettid() syscall(__NR_gettid)
 #define sigev_notify_thread_id _sigev_un._tid
 
 static volatile int done;

index 55392790..cbcb1844 100644
@@ -32,11 +32,16 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <sys/time.h>
+#ifdef __linux__
+# include <sys/syscall.h>
+#endif
 
 #include <i915/gem_submission.h>
 
 /* signal interrupt helpers */
-#define gettid() syscall(__NR_gettid)
+#ifdef __linux__
+# define gettid() syscall(__NR_gettid)
+#endif
 #define sigev_notify_thread_id _sigev_un._tid
 
 /* auxialiary igt helpers from igt_aux.c */

index 3141d923..43fa197f 100644
@@ -1992,7 +1992,7 @@ static void fatal_sig_handler(int sig)
 #ifdef __linux__
 	/* Workaround cached PID and TID races on glibc and Bionic libc. */
 		pid_t pid = syscall(SYS_getpid);
-		pid_t tid = syscall(SYS_gettid);
+		pid_t tid = gettid();
 
 		syscall(SYS_tgkill, pid, tid, sig);
 #else

index 1bdc2261..49cad7cd 100644
@@ -46,9 +46,7 @@
 #include <sys/sysmacros.h>
 #include "drm.h"
 
-#ifdef __linux__
-# include <sys/syscall.h>
-#else
+#ifndef __linux__
 # include <pthread.h>
 #endif
 

@@ -62,7 +60,7 @@ is_local_tid(pid_t tid)
 #else
 	/* On Linux systems, drmGetClient() would return the thread ID
 	   instead of the actual process ID */
-	return syscall(SYS_gettid) == tid;
+	return gettid() == tid;
 #endif
 }
 

index 4bc7b7d4..e1b0abae 100644
@@ -120,7 +120,7 @@ static void start_test(void)
 static void * test_thread(void * par)
 {
 #ifdef __linux__
-	igt_debug("start %ld\n", syscall(SYS_gettid));
+	igt_debug("start %ld\n", gettid());
 #else
 	igt_debug("start %ld\n", (long) pthread_self());
 #endif

index 11d626dc..57e00480 100644
@@ -51,7 +51,6 @@
 static uint32_t devid;
 static bool has_64bit_relocations;
 
-#define gettid() syscall(__NR_gettid)
 #define sigev_notify_thread_id _sigev_un._tid
 
 static void selfcopy(int fd, uint32_t handle, int loops)

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

Partly, for correctness. But mostly because not typecasting properly causes the gettid() macro provided by newer glibc to be typed as pid_t (aka int), while ours is typed as long. Causing annoying warnings:

[158/846] Compiling C object 'tests/59830eb@@drm_import_export@exe/drm_import_export.c.o'. In file included from ../../mnt/vol/lib/drmtest.h:39,

                 from ../../mnt/vol/lib/igt.h:27,
                 from ../../mnt/vol/tests/drm_import_export.c:27:
../../mnt/vol/tests/drm_import_export.c: In function ‘test_thread’:
../../mnt/vol/tests/drm_import_export.c:123:12: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘__pid_t’ {aka ‘int’} [-Wformat=]
  123 |  igt_debug("start %ld\n", gettid());
      |            ^~~~~~~~~~~~~  ~~~~~~~~
      |                           |
      |                           __pid_t {aka int}

../../mnt/vol/lib/igt_core.h:875:64: note: in definition of macro ‘igt_debug’
875 | #define igt_debug(f...) igt_log(IGT_LOG_DOMAIN, IGT_LOG_DEBUG, f)
| ^
../../mnt/vol/tests/drm_import_export.c:123:21: note: format string is defined here

  123 |  igt_debug("start %ld\n", gettid());
      |                   ~~^
      |                     |
      |                     long int
      |                   %d

So, typecast gettid() as pid_t and update all of our callers accordingly

---
 lib/igt_aux.h             | 3 ++-
 tests/drm_import_export.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

index cbcb1844..7dfcacb1 100644
@@ -32,6 +32,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <sys/time.h>
+#include <sys/types.h>
 #ifdef __linux__
 # include <sys/syscall.h>
 #endif

@@ -40,7 +41,7 @@
 
 /* signal interrupt helpers */
 #ifdef __linux__
-# define gettid() syscall(__NR_gettid)
+# define gettid() (pid_t)(syscall(__NR_gettid))
 #endif
 #define sigev_notify_thread_id _sigev_un._tid
 

index e1b0abae..f79c09db 100644
@@ -120,7 +120,7 @@ static void start_test(void)
 static void * test_thread(void * par)
 {
 #ifdef __linux__
-	igt_debug("start %ld\n", gettid());
+	igt_debug("start %ld\n", (long) gettid());
 #else
 	igt_debug("start %ld\n", (long) pthread_self());
 #endif

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

glibc 2.30+ will actually include a definition for gettid() that makes it so that users don't have to manually define a wrapper for it themselves with syscall(). We don't currently check for this, and as a result will end up redefining gettid() on the latest versions of glibc, causing the build to fail:

FAILED: lib/76b5a35@@igt-igt_kmod_c@sta/igt_kmod.c.o In file included from /usr/include/unistd.h:1170,

                 from ../../mnt/vol/lib/igt_core.h:43,
                 from ../../mnt/vol/lib/igt_kmod.c:28:

/usr/include/bits/unistd_ext.h:34:28: error: macro "gettid" passed 1 arguments, but takes just 0
34 | extern __pid_t gettid (void) __THROW;
| ^
In file included from ../../mnt/vol/lib/igt_kmod.c:27:
../../mnt/vol/lib/igt_aux.h:40: note: macro "gettid" defined here
40 | #define gettid() syscall(__NR_gettid)

      |

[36/771] Compiling C object 'lib/76b5a35@@igt-igt_kms_c@sta/igt_kms.c.o'.
ninja: build stopped: subcommand failed.

So, fix this by by adding some meson checks to define HAVE_GETTID whenever the host defines its own gettid(), and avoid redefining gettid() when HAVE_GETTID is defined.

This fixes build igt-gpu-tools for me on Fedora Rawhide

---
 lib/igt_aux.h | 5 ++++-
 meson.build   | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

index 7dfcacb1..d9dccbd2 100644
@@ -33,6 +33,7 @@
 #include <stddef.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <unistd.h>
 #ifdef __linux__
 # include <sys/syscall.h>
 #endif

@@ -41,7 +42,9 @@
 
 /* signal interrupt helpers */
 #ifdef __linux__
-# define gettid() (pid_t)(syscall(__NR_gettid))
+# ifndef HAVE_GETTID
+#  define gettid() (pid_t)(syscall(__NR_gettid))
+# endif
 #endif
 #define sigev_notify_thread_id _sigev_un._tid
 

index 557400a5..e73275dd 100644
@@ -237,6 +237,9 @@ if cc.has_header('cpuid.h')
 	# FIXME: Do we need the example link test from configure.ac?
 	config.set('HAVE_CPUID_H', 1)
 endif
+if cc.has_header_symbol('unistd.h', 'gettid', args : '-D_GNU_SOURCE')
+	config.set('HAVE_GETTID', 1)
+endif
 
 if cc.has_member('struct sysinfo', 'totalram',
 		prefix : '#include <sys/sysinfo.h>')

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

While I'm pretty confident that no one cares to use libigt.so or lib_aubdump.so anywhere outside of igt, many distributions including Fedora and Debian strongly suggest that packages have some sort of so versioning, even if it's just '0'. So, let's fulfill that minimum requirement to make this easier to package.

[note: still waiting on some more feedback regarding this change before I push it, another respin may or may not be imminent]

---
 lib/meson.build   | 3 ++-
 tools/meson.build | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

index 97f701c7..11ad3b1f 100644
@@ -134,7 +134,8 @@ lib_igt_build = shared_library('igt',
     ['dummy.c'],
     link_whole: lib_intermediates,
     dependencies: lib_deps,
-    install : true
+    install : true,
+    soversion : '0',
 )
 
 lib_igt = declare_dependency(link_with : lib_igt_build,

index 5d00f2e3..dfaed82a 100644
@@ -93,7 +93,8 @@ install_subdir('registers', install_dir : datadir,
 shared_library('intel_aubdump', 'aubdump.c',
 	       dependencies : [ lib_igt_chipset, dlsym ],
 	       name_prefix : '',
-	       install : true)
+	       install : true,
+	       soversion : '0')
 
 executable('intel_gpu_top', 'intel_gpu_top.c',
 	   install : true,

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

Following some discussion and confusion around whether or not assert() should be used, it seems the decision has come to "yes, sometimes". To quote Petri Latvala on the appropriate points to use assert() in lib/:

However, it's the thought that matters, and this is slightly going off on a tangent. Those uses of assert in lib/ are for places where

1) something is fatally wrong and we need to drop everything and stop executing

2) cannot use igt_assert for it. That's for places where we can say "you tried testing your kernel but it has a bug". The lib/ asserts are for "IGT has a bug", or in a couple of cases, "your IGT setup has a bug".

While we did come to the conclusion that we should possibly consider introducing a new API to check for bugs with igt (and prevent further testing if any are found), until then let's at least make sure that assert() always works where we expect it.

So, accomplsih this by raising an error if b_ndebug isn't set to 'false'. Additionally, run the compile check in lib/check-ndebug.h to make sure that the user does not have -DNDEBUG set in their CFLAGS, c_args options, etc.

---
 meson.build | 10 ++++++++++
 1 file changed, 10 insertions(+)

index e73275dd..bdf02e45 100644
@@ -3,12 +3,22 @@ project('igt-gpu-tools', 'c',
         default_options: [
           'warning_level=2',
           'c_std=gnu11',
+	  'b_ndebug=false',
         ],
 	license : 'MIT',
 	meson_version : '>=0.46.0')
 
+if get_option('b_ndebug') != 'false'
+	error('Building without -Db_ndebug=false is not supported')
+endif
+
 cc = meson.get_compiler('c')
 
+# Also make sure that the user doesn't have -DNDEBUG defined in their config
+if not cc.compiles(files('lib/check-ndebug.h'), args: get_option('c_args'))
+	error('Building with NDEBUG defined is not supported')
+endif
+
 cc_args = [
 	'-Wbad-function-cast',
 	'-Wdeclaration-after-statement',

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

That's what it's there for.

---
 lib/igt_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 43fa197f..d5d4fce2 100644
@@ -903,9 +903,7 @@ bool __igt_run_subtest(const char *subtest_name)
 {
 	int i;
 
-	assert(!in_subtest);
-	assert(!in_fixture);
-	assert(test_with_subtests);
+	assert(!igt_can_fail());
 
 	/* check the subtest name only contains a-z, A-Z, 0-9, '-' and '_' */
 	for (i = 0; subtest_name[i] != '\0'; i++)

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

dnf supports installing packages by their pkgconfig names using the pkgconfig() RPM macro. Since these more closely match the dependencies that meson uses and don't have a chance of changing in the future like Fedora package name do, let's use these with dnf instead.

---
 Dockerfile.fedora | 48 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

index 29520f7b..469e74b2 100644
@@ -1,30 +1,28 @@
 FROM fedora:28
 
-RUN dnf install -y gcc \
-		flex \
-		meson \
-		bison  \
-		gtk-doc \
-		xdotool \
-		gsl-devel \
-		kmod-devel \
-		glib2-devel \
-		cairo-devel \
-		ninja-build \
-		procps-devel \
-		pixman-devel \
-		json-c-devel \
-		libdrm-devel \
-		libudev-devel \
-		xmlrpc-c-devel \
-		elfutils-devel \
-		libunwind-devel \
-		python-docutils \
-		libpciaccess-devel \
-		alsa-lib-devel \
-		valgrind-devel \
-		libXrandr-devel \
-		libXv-devel
+RUN dnf install -y \
+	gcc flex bison meson ninja-build xdotool \
+	'pkgconfig(libdrm)' \
+	'pkgconfig(pciaccess)' \
+	'pkgconfig(libkmod)' \
+	'pkgconfig(libprocps)' \
+	'pkgconfig(libunwind)' \
+	'pkgconfig(libdw)' \
+	'pkgconfig(pixman-1)' \
+	'pkgconfig(valgrind)' \
+	'pkgconfig(cairo)' \
+	'pkgconfig(libudev)' \
+	'pkgconfig(glib-2.0)' \
+	'pkgconfig(gsl)' \
+	'pkgconfig(alsa)' \
+	'pkgconfig(xmlrpc)' \
+	'pkgconfig(xmlrpc_util)' \
+	'pkgconfig(xmlrpc_client)' \
+	'pkgconfig(json-c)' \
+	'pkgconfig(gtk-doc)' \
+	'pkgconfig(xv)' \
+	'pkgconfig(xrandr)' \
+	python3-docutils
 
 # We need peg to build overlay
 RUN dnf install -y make

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: Lyude Paul <[email protected]>

Latest stable release of Fedora

---
 Dockerfile.fedora | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 469e74b2..6cb3e2ce 100644
@@ -1,4 +1,4 @@
-FROM fedora:28
+FROM fedora:29
 
 RUN dnf install -y \
 	gcc flex bison meson ninja-build xdotool \

2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev