From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 999A73858D28 for ; Tue, 7 Dec 2021 20:33:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 999A73858D28 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-216-zpJ5mwS9OFihuW4apjAFYg-1; Tue, 07 Dec 2021 15:33:04 -0500 X-MC-Unique: zpJ5mwS9OFihuW4apjAFYg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 69FA3100CCC0; Tue, 7 Dec 2021 20:33:03 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-7.rdu2.redhat.com [10.10.112.7]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CE1E75BE06; Tue, 7 Dec 2021 20:32:58 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 1B7KWvDC3312696; Tue, 7 Dec 2021 15:32:57 -0500 From: DJ Delorie To: Alexandra =?utf-8?B?SMOhamtvdsOh?= Cc: libc-alpha@sourceware.org, mark@klomp.org, ahajkova@redhat.com Subject: Re: [PATCH] Add valgrind smoke test In-Reply-To: <20211206144043.858697-1-ahajkova@redhat.com> Date: Tue, 07 Dec 2021 15:32:57 -0500 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Dec 2021 20:33:09 -0000 Alexandra Hjkov via Libc-alpha writes: > From: Alexandra H=C3=A1jkov=C3=A1 > > Check if valgrind is present during the configure time and > run smoke tests with valgrind to verify dynamic loader. > > Co-authored-by: Mark Wielaard Legalities are OK. > --- a/elf/Makefile > +=09 valgrind-test Standard list of tests; valgrind-test.c is the "target" despite wrapping it in the script. > @@ -253,6 +254,12 @@ tests-special +=3D $(objpfx)tst-audit14-cmp.out $(ob= jpfx)tst-audit15-cmp.out \ > endif > endif > endif > + > +tests-special +=3D $(objpfx)tst-valgrind-smoke.out > +$(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so > +=09$(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' '$(run-program-env)'= '$(rpath-link)' $(objpfx)valgrind-test > $@; \ > +=09$(evaluate-test) > + This is our usual way of running a test, ok. > diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh > +#!/bin/sh Note we ignore this as we invoke it via $(SHELL) but ok > +# Valgrind smoke test. > +# Copyright (C) 2021 Free Software Foundation, Inc. > +# This file is part of the GNU C Library. > + > +# The GNU C Library is free software; you can redistribute it and/or > +# modify it under the terms of the GNU Lesser General Public > +# License as published by the Free Software Foundation; either > +# version 2.1 of the License, or (at your option) any later version. > + > +# The GNU C Library is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +# Lesser General Public License for more details. > + > +# You should have received a copy of the GNU Lesser General Public > +# License along with the GNU C Library; if not, see > +# . Ok. > +set -e > + > +rtld=3D$1 > +test_wrapper_env=3D$2 > +run_program_env=3D$3 > +library_path=3D$4 > +test_prog=3D$5 The Makefile took pains to wrap these in quotes; they should be similarly quoted here, although in reality the shell does the right thing. Not a blocker, just my parnoia ;-) > +# Test whether valgrind is available in the test > +# environment. If not, skip the test. > +${test_wrapper_env} \ > +${run_program_env} \ > +$rtld --library-path "$library_path" \ > + /bin/sh -c 'command -v valgrind' || exit 77 Tested on bash, sh, and ksh - although 1003.2-1992 doesn't list "command" as a built-in. Not a problem; we don't support systems that old ;-) We run this script via $(SHELL), we should use $(SHELL) here too, not /bin/= sh > +${test_wrapper_env} \ > +${run_program_env} \ > +$rtld --library-path "$library_path" \ > +/bin/sh -c "valgrind -q --error-exitcode=3D1 $test_prog" Same $(SHELL) here. A bit of inconsistency with $x vs ${x} vs "$x" but that doesn't affect functionality here. However, it looks like we're running /bin/sh with the just-built glibc, and the test program with the system's glibc? If you look in $(builddir)/testrun.sh you'll see how we run valgrind to test the built glibc. I.e. (in short): ${...} /bin/sh valgrind foo <- runs /bin/sh with new glibc /bin/sh ${...} valgrind foo <- runs valgrind with new glibc /bin/sh valgrind ${...} foo <- runs foo with new glibc Also, if it's your *intent* to test the system valgrind against the just-built glibc[*], that needs a comment, and should be tested in a cross-compile environment. (in general, testing in a cross-compiled case is an excellent way to see if you got the rtld stuff in the right place ;-) [*] or both valgrind and the test binary > diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c > +int > +main (void) > +{ > + setlocale (LC_ALL, ""); > + bindtextdomain ("translit", ""); > + textdomain ("translit"); > + > + return 0; > +} Any reason why these particular calls were chosen, rather than (for example) a malloc/free pair? Perhaps a bit of commentary for future readers about what valgrind is expected to be testing here...