[libcamera-devel] [PATCH v2 2/2] tests: ipa: add tests to test IPAModule

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, May 14, 2019 at 06:38:08PM -0400, Paul Elder wrote:
> Add tests to test the the IPAModule class for loading struct

"the the" ?

> IPAModuleInfo of a given symbol from a .so library.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - added source for test .so
> - updated tests to work with new (v2, see 1/2) IPAModule API
> 
>  test/ipa/ipa_test.cpp  | 76 ++++++++++++++++++++++++++++++++++++++++++
>  test/ipa/meson.build   | 23 +++++++++++++
>  test/ipa/shared_test.c | 16 +++++++++
>  test/meson.build       |  1 +
>  4 files changed, 116 insertions(+)
>  create mode 100644 test/ipa/ipa_test.cpp
>  create mode 100644 test/ipa/meson.build
>  create mode 100644 test/ipa/shared_test.c
> 
> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> new file mode 100644
> index 0000000..96849bf
> --- /dev/null
> +++ b/test/ipa/ipa_test.cpp
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * load-so.cpp - loading .so tests
> + */
> +
> +#include <iostream>
> +
> +#include "ipa_module.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class IPAModuleTest : public Test
> +{
> +protected:
> +	int init()
> +	{
> +		return 0;
> +	}
> +
> +	int run_test(const string path, const string symbol)
> +	{
> +		cout << "running lib loader test" << endl;
> +
> +		IPAModule *ll = new IPAModule(path, symbol);
> +
> +		if (!ll->isValid()) {
> +			cout << "failed to load" << endl;
> +			return TestFail;
> +		}
> +
> +		struct IPAModuleInfo info = ll->IPAModuleInfo();
> +		cout << "loaded!" << endl;
> +		cout << "name = " << info.name << ", version = " << info.version << endl;
> +
> +		delete ll;
> +		return TestPass;
> +	}
> +
> +	int run()
> +	{
> +		int count = 0;
> +
> +		cout << endl
> +		     << "testing 32-bit so from .data" << endl;
> +		count += run_test("test/ipa/libfoo.32.so", "asdf");
> +
> +		cout << endl
> +		     << "testing 32-bit so from .rodata" << endl;
> +		count += run_test("test/ipa/libfoo.32.so", "hjkl");
> +
> +		cout << endl
> +		     << "testing 64-bit so from .data" << endl;
> +		count += run_test("test/ipa/libfoo.64.so", "asdf");
> +
> +		cout << endl
> +		     << "testing 64-bit so from .rodata" << endl;
> +		count += run_test("test/ipa/libfoo.64.so", "hjkl");
> +
> +		/* Two of these should fail due to bitness mismatch. */
> +		if (count < -2)
> +			return TestFail;

Do we really need this ? Shouldn't be enough to test for success for the
correct bitness and skip the incorrect one ? We don't test specially
crafted invalid IPA modules for failure :-) (and maybe we actually
should...)

In any case, when compiling for a 32-bit platform, you won't have a
64-bit binary available, so you can't easily test incorrect bitness. I
would leave it out, at least for now.

> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +	}
> +};
> +
> +TEST_REGISTER(IPAModuleTest)
> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> new file mode 100644
> index 0000000..994d9ef
> --- /dev/null
> +++ b/test/ipa/meson.build
> @@ -0,0 +1,23 @@
> +ipa_test = [
> +    ['ipa_test',	      'ipa_test.cpp'],
> +]
> +
> +foreach t : ipa_test
> +    exe = executable(t[0], t[1],
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite: 'ipa', is_parallel: false)
> +endforeach
> +
> +libfoo_sources = files([
> +    'shared_test.c'

Can you make this a .cpp ? Or possibly test both .c and .cpp IPA modules
?

> +])
> +
> +libfoo_32 = shared_library('foo.32',
> +			   libfoo_sources,
> +			   c_args: '-m32',
> +			   link_args: '-m32')
> +
> +libfoo_64 = shared_library('foo.64',
> +			   libfoo_sources)

You don't need two separate libraries anymore, only one of them needs to
be supported. The ipa_test should depend on libfoo (I would rename it
ipa-dummy.so by the way).

> diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c
> new file mode 100644
> index 0000000..67acfa4
> --- /dev/null
> +++ b/test/ipa/shared_test.c
> @@ -0,0 +1,16 @@
> +#include <stdio.h>
> +
> +struct IPAModuleInfo {
> +	const char name[256];
> +	unsigned int version;
> +};
> +
> +const struct IPAModuleInfo hjkl = {
> +	.name = "Answer to the Ultimate Question of Life, the Universe, and Everything",
> +	.version = 42,
> +};
> +
> +struct IPAModuleInfo asdf = {
> +	.name = "It's over nine thousand!",
> +	.version = 9001,
> +};

I'm curious to know which one you will keep after hardcoding the symbol
name, as discussed in the review of 1/2 :-)

> diff --git a/test/meson.build b/test/meson.build
> index d501f2b..ef41367 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -1,6 +1,7 @@
>  subdir('libtest')
>  
>  subdir('camera')
> +subdir('ipa')
>  subdir('media_device')
>  subdir('pipeline')
>  subdir('v4l2_device')

-- 
Regards,

Laurent Pinchart