From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16304 invoked by alias); 28 Sep 2016 16:27:00 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 16267 invoked by uid 89); 28 Sep 2016 16:27:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=kit, zero_extract, Dump, locating X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 28 Sep 2016 16:26:49 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C2A6DC04B317 for ; Wed, 28 Sep 2016 16:26:48 +0000 (UTC) Received: from localhost.localdomain (ovpn-116-142.phx2.redhat.com [10.3.116.142]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8SGQmDe011112; Wed, 28 Sep 2016 12:26:48 -0400 Subject: Re: [PATCH] Introduce selftest::locate_file To: David Malcolm , gcc-patches@gcc.gnu.org References: <8c47b08f-2c65-c7b4-a056-0d51259fb4fb@redhat.com> <1474505970-4753-1-git-send-email-dmalcolm@redhat.com> From: Jeff Law Message-ID: <92f48a5e-0df7-a84c-730b-c71ff212e2b6@redhat.com> Date: Wed, 28 Sep 2016 16:33:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <1474505970-4753-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg02162.txt.bz2 On 09/21/2016 06:59 PM, David Malcolm wrote: > On Mon, 2016-09-19 at 11:31 -0600, Jeff Law wrote: >> On 09/16/2016 03:19 PM, David Malcolm wrote: >>> >>>> When possible I don't think we want the tests to be target >>>> specific. >>>> Hmm, I'm probably about to argue for Bernd's work... The 71779 >>>> testcase >>>> is a great example -- it uses high/lo_sum. Not all targets >>>> support >>>> that >>>> -- as long as we don't try to recognize those insns we're likely >>>> OK, >>>> but >>>> that seems fragile long term. Having an idealized target means >>>> we >>>> can >>>> ignore much of these issues. >>> >>> An alternative would be to pick a specific target for each test. >> It's an alternative, but not one I particularly like since those >> tests >> won't be consistently run. With an abstracted target like Bernd >> suggests we ought to be able to make most tests work with the >> abstracted >> target and minimize the number of truely target specific tests. >> >> >>>> So I'm real curious, what happens if you run this RTL selftest >>>> under >>>> valgrind? I have the sneaking suspicion that we'll start doing >>>> some >>>> uninitialized memory reads. >>> >>> I'm seeing various leaks (an htab within linemap_init for all of >>> the >>> line_table fixtures), but no uninitialized reads. >> Wow. I must say I'm surprised. Good news though. >> >> >>>> + >>>>> + /* Dump taken from comment 2 of PR 71779, of >>>>> + "...the relevant memory access coming out of expand" >>>>> + with basic block IDs added, and prev/next insns set to >>>>> + 0 at ends. */ >>>>> + const char *input_dump >>>>> + = (";; MEM[(struct isl_obj *)&obj1] = >>>>> &isl_obj_map_vtable;\n" >>>>> + "(insn 1045 0 1046 2 (set (reg:SI 480)\n" >>>>> + " (high:SI (symbol_ref:SI >>>>> (\"isl_obj_map_vtable\") >>>>> [flags 0xc0] ))) >>>>> y.c:12702 -1\n" >>>>> + " (nil))\n" >>>>> + "(insn 1046 1045 1047 2 (set (reg/f:SI 479)\n" >>>>> + " (lo_sum:SI (reg:SI 480)\n" >>>>> + " (symbol_ref:SI (\"isl_obj_map_vtable\") >>>>> [flags >>>>> 0xc0] ))) y.c:12702 >>>>> -1\n" >>>>> + " (expr_list:REG_EQUAL (symbol_ref:SI >>>>> (\"isl_obj_map_vtable\") [flags 0xc0] >>>> isl_obj_map_vtable>)\n" >>>>> + " (nil)))\n" >>>>> + "(insn 1047 1046 1048 2 (set (reg:DI 481)\n" >>>>> + " (subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1\n" >>>>> + " (nil))\n" >>>>> + "(insn 1048 1047 1049 2 (set (zero_extract:DI (reg/v:DI >>>>> 191 >>>>> [ obj1D.17368 ])\n" >>>>> + " (const_int 32 [0x20])\n" >>>>> + " (const_int 0 [0]))\n" >>>>> + " (reg:DI 481)) y.c:12702 -1\n" >>>>> + " (nil))\n" >>>> So looking at this just makes my head hurt. Escaping, lots of >>>> quotes, >>>> unnecessary things in the dump, etc. The question I would have >>>> is >>>> why >>>> not have a file with the dump and read the file? >>> >>> (nods) >>> >>> Seems like I need to add a mechanism for telling the selftests >>> which >>> directory to load the tests relative to. >> What about putting them inside the appropriate gcc.target >> directories? >> We could have one for the "generic" target assuming we build >> something >> like that, the others could live in their target specific directory. >> >> >> jeff > > Having selftests that read RTL dumps load them from files rather than > embedding them as strings in the source requires a way to locate the > relevant files. > > This patch adds a selftest::locate_file function for locating such > files, relative to "$(SRCDIR)/gcc/testsuite/selftests". This is > done via a new argument to -fself-test, which supplies the current > value of "$(SRCDIR)/gcc" to cc1. > > I chose "$(SRCDIR)/gcc/testsuite/selftests", so as to be below > gcc/testsuite, but not below any of the existing DejaGnu subdirectories, > to avoid selftest-specific files from being picked up by .exp globbing > patterns. We could add target-specific directories below that dir if > necessary. > > I've rewritten the rest of the patch kit to use this to load from .rtl > dump files within that directory, rather than embedding the dumps as > string literals in the C source. > > The patch also exposes a selftests::path_to_src_gcc, which could be > used by a selftest to e.g. load a DejaGnu file, so that if need be > we could share .rtl input files between both -fself-test tests and > DejaGnu-based tests for the .rtl frontend. > > (Depends on the approved-when-needed > "[PATCH 2/9] Add selftest::read_file" > https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00476.html ). > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > OK for trunk once the dependencies are in? > > gcc/ChangeLog: > * Makefile.in (s-selftest) Add $(srcdir) as an argument of > -fself-test. > (selftest-gdb): Likewise. > (selftest-valgrind): Likewise. > * common.opt (fself-test): Rename to... > (fself-test=): ...this, documenting the meaning of the argument. > * selftest-run-tests.c: Include "options.h". > (selftest::run_tests): Initialize selftest::path_to_src_gcc from > flag_self_test. > * selftest.c (selftest::path_to_src_gcc): New global. > (selftest::locate_file): New function. > (selftest::test_locate_file): New function. > (selftest::selftest_c_tests): Call test_locate_file. > * selftest.h (selftest::locate_file): New decl. > (selftest::path_to_src_gcc): New decl. > > gcc/testsuite/ChangeLog: > * gcc.dg/cpp/pr71591.c: Add a fake value for the argument of > -fself-test. > * selftests/example.txt: New file. I do think we should go ahead and plan for a target subdirectory. With that added, this should be OK once dependencies are in. jeff