[{"id":16169,"web_url":"https://patchwork.libcamera.org/comment/16169/","msgid":"<87c148e2-6a9c-062e-c913-16fa9b191ac6@ideasonboard.com>","date":"2021-04-12T04:15:00","subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 4/12/21 3:06 AM, Laurent Pinchart wrote:\n> The last instance of Thread allocated in the test is never deleted, not\nI think you meant s/not/nor\n> are other instances deleted in error paths. Use a std::unique_ptr<> to\n> ensure deletion.\n>\n> Signed-off-by: Laurent Pinchart \nReviewed-by: Umang Jain \n\n> ---\n> test/threads.cpp | 19 ++++++++-----------\n> 1 file changed, 8 insertions(+), 11 deletions(-)\n>\n> diff --git a/test/threads.cpp b/test/threads.cpp\n> index b4b8d913cd2b..e0c50dc90a65 100644\n> --- a/test/threads.cpp\n> +++ b/test/threads.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \"libcamera/internal/thread.h\"\n> @@ -45,23 +46,23 @@ protected:\n> \tint run()\n> \t{\n> \t\t/* Test Thread() retrieval for the main thread. */\n> -\t\tThread *thread = Thread::current();\n> -\t\tif (!thread) {\n> +\t\tThread *mainThread = Thread::current();\n> +\t\tif (!mainThread) {\n> \t\t\tcout << \"Thread::current() failed to main thread\"\n> \t\t\t << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tif (!thread->isRunning()) {\n> +\t\tif (!mainThread->isRunning()) {\n> \t\t\tcout << \"Main thread is not running\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> \t\t/* Test starting the main thread, the test shall not crash. */\n> -\t\tthread->start();\n> +\t\tmainThread->start();\n> \n> \t\t/* Test the running state of a custom thread. */\n> -\t\tthread = new Thread();\n> +\t\tstd::unique_ptr thread = std::make_unique();\n> \t\tthread->start();\n> \n> \t\tif (!thread->isRunning()) {\n> @@ -79,10 +80,8 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tdelete thread;\n> -\n> \t\t/* Test waiting for completion with a timeout. */\n> -\t\tthread = new DelayThread(chrono::milliseconds(500));\n> +\t\tthread = std::make_unique(chrono::milliseconds(500));\n> \t\tthread->start();\n> \t\tthread->exit(0);\n> \n> @@ -100,10 +99,8 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tdelete thread;\n> -\n> \t\t/* Test waiting on a thread that isn't running. */\n> -\t\tthread = new Thread();\n> +\t\tthread = std::make_unique();\n> \n> \t\ttimeout = !thread->wait();\n> \t\tif (timeout) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E6D34BD1F6\n\tfor ;\n\tMon, 12 Apr 2021 04:15:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33DB4602C8;\n\tMon, 12 Apr 2021 06:15:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 570BE602C8\n\tfor ;\n\tMon, 12 Apr 2021 06:15:06 +0200 (CEST)","from localhost.localdomain (unknown [103.251.226.175])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 105263F0;\n\tMon, 12 Apr 2021 06:15:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CMJATfS3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618200905;\n\tbh=N9KG2Gf2NOmV/eaBPE+WiWU4ZmgEckNYplVe0EsXsec=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=CMJATfS3f6vzmZLUZiZ2XeaeSecrH73AFXklZiB/s7hoFFijCJn++mGcrR24K9C4x\n\tlMHC9BO5gyVb+AvyxZVgb2R3/ZKaGxBRSruWeFWp3JlAO06+MmwQYlbeuHulafjKrt\n\tVQWH5VZtaEqPjNaFk2KDgOOWXXTkPnLrtamR5LiU=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210411213607.9451-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<87c148e2-6a9c-062e-c913-16fa9b191ac6@ideasonboard.com>","Date":"Mon, 12 Apr 2021 09:45:00 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210411213607.9451-1-laurent.pinchart@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16170,"web_url":"https://patchwork.libcamera.org/comment/16170/","msgid":"<20210412042136.qjg2cziahr4aq6zn@basti-TUXEDO-Book-XA1510>","date":"2021-04-12T04:21:36","subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","submitter":{"id":78,"url":"https://patchwork.libcamera.org/api/people/78/","name":"Sebastian Fricke","email":"sebastian.fricke@posteo.net"},"content":"Hey Laurent,\n\nThank you for the patch.\n\nOn 12.04.2021 00:36, Laurent Pinchart wrote:\n>The last instance of Thread allocated in the test is never deleted, not\n\ns/not/nor/ ?\n\n>are other instances deleted in error paths. Use a std::unique_ptr<> to\n>ensure deletion.\n\nI tested this fix and valgrind does not report any leaked memory from\nthe libcamera tests. But just out of curiosity do you get a similar\nreport in valgrind:\n```\nbasti@nanopct4:~/libcamera$ valgrind --leak-check=full --show-leak-kinds=all ninja -C build test\n...\nlibcamera 0.0.0\n\n Configuration\n Enabled pipelines : ipu3\n raspberrypi\n rkisp1\n simple\n uvcvideo\n vimc\n Android support : False\n GStreamer support : False\n V4L2 emulation support: False\n cam application : True\n qcam application : False\n Unit tests : True\n\nOption werror is: true [default: true]\n...\n==6151== LEAK SUMMARY:\n==6151== definitely lost: 296 bytes in 1 blocks\n==6151== indirectly lost: 576 bytes in 2 blocks\n==6151== possibly lost: 0 bytes in 0 blocks\n==6151== still reachable: 2,725,231 bytes in 15,220 blocks\n==6151== suppressed: 0 bytes in 0 blocks\n```\n\nThose are all related to our build tool ninja.\n\n>\n>Signed-off-by: Laurent Pinchart \nTested-by: Sebastian Fricke \n\nGreetings,\nSebastian\n\n>---\n> test/threads.cpp | 19 ++++++++-----------\n> 1 file changed, 8 insertions(+), 11 deletions(-)\n>\n>diff --git a/test/threads.cpp b/test/threads.cpp\n>index b4b8d913cd2b..e0c50dc90a65 100644\n>--- a/test/threads.cpp\n>+++ b/test/threads.cpp\n>@@ -7,6 +7,7 @@\n>\n> #include \n> #include \n>+#include \n> #include \n>\n> #include \"libcamera/internal/thread.h\"\n>@@ -45,23 +46,23 @@ protected:\n> \tint run()\n> \t{\n> \t\t/* Test Thread() retrieval for the main thread. */\n>-\t\tThread *thread = Thread::current();\n>-\t\tif (!thread) {\n>+\t\tThread *mainThread = Thread::current();\n>+\t\tif (!mainThread) {\n> \t\t\tcout << \"Thread::current() failed to main thread\"\n> \t\t\t << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n>-\t\tif (!thread->isRunning()) {\n>+\t\tif (!mainThread->isRunning()) {\n> \t\t\tcout << \"Main thread is not running\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> \t\t/* Test starting the main thread, the test shall not crash. */\n>-\t\tthread->start();\n>+\t\tmainThread->start();\n>\n> \t\t/* Test the running state of a custom thread. */\n>-\t\tthread = new Thread();\n>+\t\tstd::unique_ptr thread = std::make_unique();\n> \t\tthread->start();\n>\n> \t\tif (!thread->isRunning()) {\n>@@ -79,10 +80,8 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n>-\t\tdelete thread;\n>-\n> \t\t/* Test waiting for completion with a timeout. */\n>-\t\tthread = new DelayThread(chrono::milliseconds(500));\n>+\t\tthread = std::make_unique(chrono::milliseconds(500));\n> \t\tthread->start();\n> \t\tthread->exit(0);\n>\n>@@ -100,10 +99,8 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n>-\t\tdelete thread;\n>-\n> \t\t/* Test waiting on a thread that isn't running. */\n>-\t\tthread = new Thread();\n>+\t\tthread = std::make_unique();\n>\n> \t\ttimeout = !thread->wait();\n> \t\tif (timeout) {\n>-- \n>Regards,\n>\n>Laurent Pinchart\n>\n>_______________________________________________\n>libcamera-devel mailing list\n>libcamera-devel@lists.libcamera.org\n>https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97436BD224\n\tfor ;\n\tMon, 12 Apr 2021 04:21:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E792687F3;\n\tMon, 12 Apr 2021 06:21:40 +0200 (CEST)","from mout01.posteo.de (mout01.posteo.de [185.67.36.65])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39379602C8\n\tfor ;\n\tMon, 12 Apr 2021 06:21:38 +0200 (CEST)","from submission (posteo.de [89.146.220.130]) \n\tby mout01.posteo.de (Postfix) with ESMTPS id 7CC061A022B\n\tfor ;\n\tMon, 12 Apr 2021 06:21:37 +0200 (CEST)","from customer (localhost [127.0.0.1])\n\tby submission (posteo.de) with ESMTPSA id 4FJbCh6nQXz9rxF;\n\tMon, 12 Apr 2021 06:21:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=posteo.net header.i=@posteo.net\n\theader.b=\"imA0NbQD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;\n\tt=1618201297; bh=F8PkiGaeAX/Pjrbu7xOYuII80Kpc6y0bHxv5YjEH07M=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=imA0NbQDw97Ri2qhJij9ieqLTbmFbLxDtC+sue6W5vK6r1irvqWiUJa964QLR650g\n\tNUWY5TGE9q2KUrVkjDbN3K6I8eiSyGVOrVqEIasempVtCtxh6B4PLD+1JZ4aCl24t8\n\ti1gAn/8F49zMLYpE7Ov9gWlVm/JFnvfmV2Ymk+YFjWsX56bZUXZRQuXaOH1SK2vgX6\n\t4SgGdiBBN0hPdFk9EjL9qdGSrOM9q77PPIDJgP+SgTZ8pkrsyG6H3CH7B7DlgRvhSq\n\tww92M8BNk4/qACu13eqIbipakgDhD2xClbbObV2egFmi1/6WMdkEjieVWGea6lhGzp\n\tFBp5AYrs55eag==","Date":"Mon, 12 Apr 2021 06:21:36 +0200","From":"Sebastian Fricke ","To":"Laurent Pinchart ","Message-ID":"<20210412042136.qjg2cziahr4aq6zn@basti-TUXEDO-Book-XA1510>","References":"<20210411213607.9451-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210411213607.9451-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16171,"web_url":"https://patchwork.libcamera.org/comment/16171/","msgid":"","date":"2021-04-12T07:24:02","subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sebastian,\n\nOn Mon, Apr 12, 2021 at 06:21:36AM +0200, Sebastian Fricke wrote:\n> Hey Laurent,\n> \n> Thank you for the patch.\n> \n> On 12.04.2021 00:36, Laurent Pinchart wrote:\n> > The last instance of Thread allocated in the test is never deleted, not\n> \n> s/not/nor/ ?\n\nOops, indeed.\n\n> > are other instances deleted in error paths. Use a std::unique_ptr<> to\n> > ensure deletion.\n> \n> I tested this fix and valgrind does not report any leaked memory from\n> the libcamera tests. But just out of curiosity do you get a similar\n> report in valgrind:\n> ```\n> basti@nanopct4:~/libcamera$ valgrind --leak-check=full --show-leak-kinds=all ninja -C build test\n> ...\n> libcamera 0.0.0\n> \n> Configuration\n> Enabled pipelines : ipu3\n> raspberrypi\n> rkisp1\n> simple\n> uvcvideo\n> vimc\n> Android support : False\n> GStreamer support : False\n> V4L2 emulation support: False\n> cam application : True\n> qcam application : False\n> Unit tests : True\n> \n> Option werror is: true [default: true]\n> ...\n> ==6151== LEAK SUMMARY:\n> ==6151== definitely lost: 296 bytes in 1 blocks\n> ==6151== indirectly lost: 576 bytes in 2 blocks\n> ==6151== possibly lost: 0 bytes in 0 blocks\n> ==6151== still reachable: 2,725,231 bytes in 15,220 blocks\n> ==6151== suppressed: 0 bytes in 0 blocks\n> ```\n> \n> Those are all related to our build tool ninja.\n\nThat's checking for leaks in ninja itself, it's not ours.\n\n> > Signed-off-by: Laurent Pinchart \n>\n> Tested-by: Sebastian Fricke \n> \n> > ---\n> > test/threads.cpp | 19 ++++++++-----------\n> > 1 file changed, 8 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/test/threads.cpp b/test/threads.cpp\n> > index b4b8d913cd2b..e0c50dc90a65 100644\n> > --- a/test/threads.cpp\n> > +++ b/test/threads.cpp\n> > @@ -7,6 +7,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \"libcamera/internal/thread.h\"\n> > @@ -45,23 +46,23 @@ protected:\n> > \tint run()\n> > \t{\n> > \t\t/* Test Thread() retrieval for the main thread. */\n> > -\t\tThread *thread = Thread::current();\n> > -\t\tif (!thread) {\n> > +\t\tThread *mainThread = Thread::current();\n> > +\t\tif (!mainThread) {\n> > \t\t\tcout << \"Thread::current() failed to main thread\"\n> > \t\t\t << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > -\t\tif (!thread->isRunning()) {\n> > +\t\tif (!mainThread->isRunning()) {\n> > \t\t\tcout << \"Main thread is not running\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > \t\t/* Test starting the main thread, the test shall not crash. */\n> > -\t\tthread->start();\n> > +\t\tmainThread->start();\n> > \n> > \t\t/* Test the running state of a custom thread. */\n> > -\t\tthread = new Thread();\n> > +\t\tstd::unique_ptr thread = std::make_unique();\n> > \t\tthread->start();\n> > \n> > \t\tif (!thread->isRunning()) {\n> > @@ -79,10 +80,8 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > -\t\tdelete thread;\n> > -\n> > \t\t/* Test waiting for completion with a timeout. */\n> > -\t\tthread = new DelayThread(chrono::milliseconds(500));\n> > +\t\tthread = std::make_unique(chrono::milliseconds(500));\n> > \t\tthread->start();\n> > \t\tthread->exit(0);\n> > \n> > @@ -100,10 +99,8 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > -\t\tdelete thread;\n> > -\n> > \t\t/* Test waiting on a thread that isn't running. */\n> > -\t\tthread = new Thread();\n> > +\t\tthread = std::make_unique();\n> > \n> > \t\ttimeout = !thread->wait();\n> > \t\tif (timeout) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 98CB2BD1F6\n\tfor ;\n\tMon, 12 Apr 2021 07:24:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EED32687F5;\n\tMon, 12 Apr 2021 09:24:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4641F687F5\n\tfor ;\n\tMon, 12 Apr 2021 09:24:53 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B39D3F0;\n\tMon, 12 Apr 2021 09:24:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ofe4JBE1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618212292;\n\tbh=bQ5/Fx58hd/mNlfxItuLoYvQLKx/R2DU3MFrsyIbzP0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ofe4JBE1wSbk9FC9dwE0qweWKxoE4W1hScdkHHKC/1QB8O95Arf0piUhBwa+GvUec\n\tpUwbTIRFcEOHLsJ5RepI24iy21zPCsQfGoG/5N3HgM9/9sr/9bFWVXQXbCSgOkvsuy\n\t3btXZ4Ez6wySl+tbNyRrtAVxUewKQvhNp4RzaCyE=","Date":"Mon, 12 Apr 2021 10:24:02 +0300","From":"Laurent Pinchart ","To":"Sebastian Fricke ","Message-ID":"","References":"<20210411213607.9451-1-laurent.pinchart@ideasonboard.com>\n\t<20210412042136.qjg2cziahr4aq6zn@basti-TUXEDO-Book-XA1510>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210412042136.qjg2cziahr4aq6zn@basti-TUXEDO-Book-XA1510>","Subject":"Re: [libcamera-devel] [PATCH] test: threads: Fix memory leak","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]