Re: [igt-dev] [PATCH i-g-t 3/7] lib: Stop using assert() for runtime checks

Petri Latvala
2019-04-24T10:13:46Z

On Tue, Apr 23, 2019 at 08:36:43PM -0400, Lyude Paul wrote:
> On Wed, 2019-04-17 at 11:52 +0300, Petri Latvala wrote:
> > On Tue, Apr 16, 2019 at 04:10:57PM -0400, Lyude wrote:
> > > From: Lyude Paul <lyude@redhat.com>
> > >
> > > In the process of trying to get an up to date version of igt packaged
> > > for Fedora I discovered that if igt was built with -Db_ndebug=true, a
> > > significant portion of the test infrastructure unit tests would start
> > > failing:
> > >
> > > 1/265 lib: igt_assert OK 0.11 s
> > > 2/265 lib: igt_can_fail OK 0.08 s
> > > 3/265 lib: igt_can_fail_simple OK 0.08 s
> > > 4/265 lib: igt_exit_handler OK 0.05 s
> > > 5/265 lib: igt_fork FAIL 0.05 s (killed by
> > > signal 9 SIGKILL)
> > > 6/265 lib: igt_fork_helper OK 0.42 s
> > > 7/265 lib: igt_hdmi_inject OK 0.05 s
> > > 8/265 lib: igt_list_only OK 0.01 s
> > > 9/265 lib: igt_invalid_subtest_name OK 0.05 s
> > > 10/265 lib: igt_no_exit OK 0.04 s
> > > 11/265 lib: igt_segfault OK 0.38 s
> > > 12/265 lib: igt_simulation OK 0.02 s
> > > 13/265 lib: igt_stats OK 0.04 s
> > > 14/265 lib: igt_subtest_group OK 0.03 s
> > > 15/265 lib: igt_no_subtest SKIP 0.02 s
> > > 16/265 lib: igt_simple_test_subtests UNEXPECTEDPASS 0.02 s
> > > 17/265 lib: igt_timeout EXPECTEDFAIL 1.02 s
> > >
> > > Which appeared to stem from the fact that -Db_ndebug=true would strip
> > > assert() calls. While on a first glance of lib/tests/igt_tests_common.h
> > > one would assume that the only user of assert() was the test
> > > infrastructure unit tests themselves, it turns out we've actually been
> > > using this in multiple spots that seem to expect an unconditional
> > > runtime check.
> >
> > Umm, yes. You've uncovered a bug, but not what you think. With meson,
> > we don't include lib/check-ndebug.h anymore, only with autotools so
> > you missed the important part: Do not attempt to build IGT with
> > b_ndebug=true.
>
> If igt really isn't supposed to be built with -Db_ndebug=true we really should
> add explicit checks for this in our meson.build file, e.g.
>
> if get_option("b_ndebug") == true
> error("Building with -Db_ndebug=true is not supported")
> endif
>
> Especially since the default in meson upstream is going to be changing to having
> -Db_ndebug=true on release builds pretty soon. I'll include a patch for this in
> the v3 respin.

Yep, an explicit check sounds good. And overriding the default in the
project() call in toplevel meson.build. And having lib/check-ndebug.h
used in meson builds is a TODO item as well, in case people have a
manual -DNDEBUG in their cflags.

--
Petri Latvala