[{"id":1601,"web_url":"https://patchwork.libcamera.org/comment/1601/","msgid":"<20190515212617.GB4991@pendragon.ideasonboard.com>","date":"2019-05-15T21:26:17","subject":"Re: [libcamera-devel] [PATCH v2 2/2] tests: ipa: add tests to test\n\tIPAModule","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, May 14, 2019 at 06:38:08PM -0400, Paul Elder wrote:\n> Add tests to test the the IPAModule class for loading struct\n\n\"the the\" ?\n\n> IPAModuleInfo of a given symbol from a .so library.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - added source for test .so\n> - updated tests to work with new (v2, see 1/2) IPAModule API\n> \n> test/ipa/ipa_test.cpp | 76 ++++++++++++++++++++++++++++++++++++++++++\n> test/ipa/meson.build | 23 +++++++++++++\n> test/ipa/shared_test.c | 16 +++++++++\n> test/meson.build | 1 +\n> 4 files changed, 116 insertions(+)\n> create mode 100644 test/ipa/ipa_test.cpp\n> create mode 100644 test/ipa/meson.build\n> create mode 100644 test/ipa/shared_test.c\n> \n> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> new file mode 100644\n> index 0000000..96849bf\n> --- /dev/null\n> +++ b/test/ipa/ipa_test.cpp\n> @@ -0,0 +1,76 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * load-so.cpp - loading .so tests\n> + */\n> +\n> +#include \n> +\n> +#include \"ipa_module.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class IPAModuleTest : public Test\n> +{\n> +protected:\n> +\tint init()\n> +\t{\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run_test(const string path, const string symbol)\n> +\t{\n> +\t\tcout << \"running lib loader test\" << endl;\n> +\n> +\t\tIPAModule *ll = new IPAModule(path, symbol);\n> +\n> +\t\tif (!ll->isValid()) {\n> +\t\t\tcout << \"failed to load\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tstruct IPAModuleInfo info = ll->IPAModuleInfo();\n> +\t\tcout << \"loaded!\" << endl;\n> +\t\tcout << \"name = \" << info.name << \", version = \" << info.version << endl;\n> +\n> +\t\tdelete ll;\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\tint count = 0;\n> +\n> +\t\tcout << endl\n> +\t\t << \"testing 32-bit so from .data\" << endl;\n> +\t\tcount += run_test(\"test/ipa/libfoo.32.so\", \"asdf\");\n> +\n> +\t\tcout << endl\n> +\t\t << \"testing 32-bit so from .rodata\" << endl;\n> +\t\tcount += run_test(\"test/ipa/libfoo.32.so\", \"hjkl\");\n> +\n> +\t\tcout << endl\n> +\t\t << \"testing 64-bit so from .data\" << endl;\n> +\t\tcount += run_test(\"test/ipa/libfoo.64.so\", \"asdf\");\n> +\n> +\t\tcout << endl\n> +\t\t << \"testing 64-bit so from .rodata\" << endl;\n> +\t\tcount += run_test(\"test/ipa/libfoo.64.so\", \"hjkl\");\n> +\n> +\t\t/* Two of these should fail due to bitness mismatch. */\n> +\t\tif (count < -2)\n> +\t\t\treturn TestFail;\n\nDo we really need this ? Shouldn't be enough to test for success for the\ncorrect bitness and skip the incorrect one ? We don't test specially\ncrafted invalid IPA modules for failure :-) (and maybe we actually\nshould...)\n\nIn any case, when compiling for a 32-bit platform, you won't have a\n64-bit binary available, so you can't easily test incorrect bitness. I\nwould leave it out, at least for now.\n\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(IPAModuleTest)\n> diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> new file mode 100644\n> index 0000000..994d9ef\n> --- /dev/null\n> +++ b/test/ipa/meson.build\n> @@ -0,0 +1,23 @@\n> +ipa_test = [\n> + ['ipa_test',\t 'ipa_test.cpp'],\n> +]\n> +\n> +foreach t : ipa_test\n> + exe = executable(t[0], t[1],\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite: 'ipa', is_parallel: false)\n> +endforeach\n> +\n> +libfoo_sources = files([\n> + 'shared_test.c'\n\nCan you make this a .cpp ? Or possibly test both .c and .cpp IPA modules\n?\n\n> +])\n> +\n> +libfoo_32 = shared_library('foo.32',\n> +\t\t\t libfoo_sources,\n> +\t\t\t c_args: '-m32',\n> +\t\t\t link_args: '-m32')\n> +\n> +libfoo_64 = shared_library('foo.64',\n> +\t\t\t libfoo_sources)\n\nYou don't need two separate libraries anymore, only one of them needs to\nbe supported. The ipa_test should depend on libfoo (I would rename it\nipa-dummy.so by the way).\n\n> diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c\n> new file mode 100644\n> index 0000000..67acfa4\n> --- /dev/null\n> +++ b/test/ipa/shared_test.c\n> @@ -0,0 +1,16 @@\n> +#include \n> +\n> +struct IPAModuleInfo {\n> +\tconst char name[256];\n> +\tunsigned int version;\n> +};\n> +\n> +const struct IPAModuleInfo hjkl = {\n> +\t.name = \"Answer to the Ultimate Question of Life, the Universe, and Everything\",\n> +\t.version = 42,\n> +};\n> +\n> +struct IPAModuleInfo asdf = {\n> +\t.name = \"It's over nine thousand!\",\n> +\t.version = 9001,\n> +};\n\nI'm curious to know which one you will keep after hardcoding the symbol\nname, as discussed in the review of 1/2 :-)\n\n> diff --git a/test/meson.build b/test/meson.build\n> index d501f2b..ef41367 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -1,6 +1,7 @@\n> subdir('libtest')\n> \n> subdir('camera')\n> +subdir('ipa')\n> subdir('media_device')\n> subdir('pipeline')\n> subdir('v4l2_device')","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 270CA60103\n\tfor ;\n\tWed, 15 May 2019 23:26:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F5B4443;\n\tWed, 15 May 2019 23:26:33 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557955593;\n\tbh=kXkThL6vL/iM9PhHbOj9drQvPIWE/Y3YL3eVOWshRuQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VLM7PPY0fRKNeThTsH6iTz19Ro8t60mfxvl0FXk9zOL2+epCoTTVlxEUYQh1T+z9g\n\tuNMwesrEoNpjV0DxcfSPiqRJ3es4n7XKK/VHhNN5UulT5q2qz9+MRQaUbOdpST+qSZ\n\tW58Ib9BDozFNLqcDmDjKR1Qf2nv5ees+rbfC9x3Y=","Date":"Thu, 16 May 2019 00:26:17 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190515212617.GB4991@pendragon.ideasonboard.com>","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<20190514223808.27351-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190514223808.27351-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] tests: ipa: add tests to test\n\tIPAModule","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 15 May 2019 21:26:34 -0000"}}]