Re: [PATCH] Introduce selftest::locate_file

[Jeff Law <law@redhat.com>]

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] <var_decl 0x7fa0363ea240 isl_obj_map_vtable>)))
>>>>> 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] <var_decl 0x7fa0363ea240 isl_obj_map_vtable>))) y.c:12702
>>>>> -1\n"
>>>>> +       "     (expr_list:REG_EQUAL (symbol_ref:SI
>>>>> (\"isl_obj_map_vtable\") [flags 0xc0] <var_decl 0x7fa0363ea240
>>>>> 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&regrtested 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