[5/9] Introduce selftest::locate_file (v4)

Link to discussion of earlier version of patch:
  https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00260.html

On Wed, 2016-10-05 at 18:10 +0200, Bernd Schmidt wrote:
> On 10/05/2016 06:15 PM, David Malcolm wrote:
> >  selftest: s-selftest
> >  s-selftest: $(GCC_PASSES) cc1$(exeext) stmp-int-hdrs
> > -	$(GCC_FOR_TARGET) -xc -S -c /dev/null -fself-test
> > +	$(GCC_FOR_TARGET) -xc -S -c /dev/null -fself
> > -test=$(srcdir)
> >  	$(STAMP) $@
>
> I suspect the Makefile parts will need updating once Thomas commits
> his
> change.

Updated.

> Also,
>
> > +  return concat (path_to_src_gcc, "/testsuite/selftests/", name,
> > NULL);
>
> why hardcode directory names here? I think this should just be passed
> as
> a full pathname so only the Makefiles have knowledge of source
> directory
> structure.

Done, renaming path_to_src_gcc to path_to_selftest_files.

I also added a dep on selftest data dir to s-selftests in Makefile.in, so
that edits to the data mean re-running the selftests.

gcc/ChangeLog:
	* Makefile.in (SELFTEST_FLAGS): Add path argument to -fself-test.
	(s-selftest): Add dependency on the selftests data directory.
	* common.opt (fself-test): Rename to...
	(fself-test=): ...this, documenting the meaning of the argument.
	* selftest-run-tests.c (along): Likewise.
	* selftest-run-tests.c: Include "options.h".
	(selftest::run_tests): Initialize selftest::path_to_selftest_files
	from flag_self_test.
	* selftest.c (selftest::path_to_selftest_files): New global.
	(selftest::locate_file): New function.
	(selftest::test_locate_file): New function.
	(selftest_c_tests): Likewise.
	(selftest::selftest_c_tests): Call test_locate_file.
	* selftest.h (selftest::locate_file): New decl.
	(selftest::path_to_selftest_files): 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.
---
 gcc/Makefile.in                     | 10 +++++++---
 gcc/common.opt                      |  6 +++---
 gcc/selftest-run-tests.c            |  8 ++++++++
 gcc/selftest.c                      | 28 ++++++++++++++++++++++++++++
 gcc/selftest.h                      | 10 ++++++++++
 gcc/testsuite/gcc.dg/cpp/pr71591.c  |  2 +-
 gcc/testsuite/selftests/example.txt |  1 +
 7 files changed, 58 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/selftests/example.txt

On 11/11/2016 10:15 PM, David Malcolm wrote:
> +  /* Makefile.in has -fself-test=$(srcdir)/testsuite/selftests, so that
> +     flag_self_test contains the path to the selftest subdirectory of the
> +     source tree (without a trailing slash).  Copy it up to
> +     path_to_selftest_files, to avoid selftest.c depending on
> +     option-handling.  */
> +  path_to_selftest_files = flag_self_test;
> +

What kind of dependency are you avoiding? If it's just one include I'd 
prefer to get rid of the extraneous variable.

Otherwise ok.


Bernd

On Thu, 2016-12-01 at 14:29 +0100, Bernd Schmidt wrote:
> On 11/11/2016 10:15 PM, David Malcolm wrote:
> > +  /* Makefile.in has -fself-test=$(srcdir)/testsuite/selftests, so
> > that
> > +     flag_self_test contains the path to the selftest subdirectory
> > of the
> > +     source tree (without a trailing slash).  Copy it up to
> > +     path_to_selftest_files, to avoid selftest.c depending on
> > +     option-handling.  */
> > +  path_to_selftest_files = flag_self_test;
> > +
> 
> What kind of dependency are you avoiding? If it's just one include
> I'd 
> prefer to get rid of the extraneous variable.

I was thinking more about keeping selftest.h/c modularized; I didn't
want them knowing anything about gcc option-handling, in case we want
to move the core of the selftests into some other support directory
(libiberty or similar).

Perhaps a better approach is to add the path as a param to locate_file,
to use it from callers.  That would support multiple subprojects using
selftest::locate_file (avoiding a global variable), at the slight cost
of requiring the use of flag_self_test (actually a macro) everywhere in
gcc that we use locate_file.

Should I update the patches to reflect that?

> Otherwise ok.
> 
> 
> Bernd
>

On Thu, 2016-12-01 at 14:29 +0100, Bernd Schmidt wrote:
> On 11/11/2016 10:15 PM, David Malcolm wrote:
> > +  /* Makefile.in has -fself-test=$(srcdir)/testsuite/selftests, so
> > that
> > +     flag_self_test contains the path to the selftest subdirectory
> > of the
> > +     source tree (without a trailing slash).  Copy it up to
> > +     path_to_selftest_files, to avoid selftest.c depending on
> > +     option-handling.  */
> > +  path_to_selftest_files = flag_self_test;
> > +
> 
> What kind of dependency are you avoiding? If it's just one include
> I'd 
> prefer to get rid of the extraneous variable.
> 
> Otherwise ok.

I attempted to eliminate the global, and include "options.h" from
selftest.c, and to use flag_self_test directly (implicitly accessing
the global_options global).

The *code* changes were relatively simple, with only one extra
#include.

However, from a linker perspective, it's more awkward.  selftest.o is
used by (amongst other things) OBJS-libcommon: libcommon.a currently
has selftest.o, but doesn't have options.o.  Various things in
libcommon.a use selftest.o for selftests and thus libcommon.a needs
selftest.o.

Hence doing so brings in a dependency on the option-handling objects
into libcommon.a, which seems wrong to me.

It seems simplest to go with either:

(A) the approach in the patch, with an extra global, keeping the
selftest code independent from gcc's option-handling code (both at
compile time and link time) or

(B) add the path as an extra param, passing it in at all callsites,
so that every
   locate_file (some_path)
in the kit becomes
   locate_file (flag_self_test, some_path)

Is (A) OK, or would you prefer (B)?   (I prefer (A) fwiw)

Thanks
Dave

On 12/08/2016 10:47 PM, David Malcolm wrote:

> Is (A) OK, or would you prefer (B)?   (I prefer (A) fwiw)

Oh well, just keep it as it is.


Bernd