Re: [PATCH] Introduce selftest::locate_file
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] )\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.
(selfte
[PATCH] Introduce selftest::locate_file
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
