Re: [PATCH, Modula-2 (C/C++/D/F/Go/Jit)] (Register spec fn) (v2)

[Richard Sandiford <richard.sandiford@arm.com>]

Thanks for the patch and sorry for the slow reply.

Gaius Mulley <gaius.mulley@southwales.ac.uk> writes:
> Hello,
>
> here is version two of the patches which introduce Modula-2 into the
> GCC trunk.  The patches include:
>
>   (*)  a patch to allow all front ends to register a lang spec function.
>        (included are patches for all front ends to provide an empty
>         callback function).
>   (*)  patch diffs to allow the Modula-2 front end driver to be
>        built using GCC Makefile and friends.
>
> The compressed tarball includes:
>
>   (*)  gcc/m2  (compiler driver and lang-spec stuff for Modula-2).
>        Including the need for registering lang spec functions.
>   (*)  gcc/testsuite/gm2  (a Modula-2 dejagnu test to ensure that
>        the gm2 driver is built and can understands --version).
>
> These patches have been re-written after taking on board the comments
> found in this thread:
>
>    https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02620.html

Echoing a point Joseph made there: does this approach to linking work
with LTO, especially given the auto-generated main module?

I don't think that should hold up the patch, just curious.

> it is a revised patch set from:
>
>    https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00220.html
>
> I've run make bootstrap and run the regression tests on trunk and no
> extra failures occur for all languages touched in the ChangeLog.
>
> I'm currently tracking gcc trunk and gcc-9 with gm2 (which works well
> with amd64/arm64/i386) - these patches are currently simply for the
> driver to minimise the patch size.  There are also > 1800 tests in a
> dejagnu testsuite for gm2 which can be included at some future time.

It looks like the current GCC driver code is a bit of a half-transition
towards a more C++ way of doing things.  If we were going to embrace
that fully, I guess each frontend driver would get the opportunity
to subclass "driver" and override functions where appropriate.  It then
wouldn't be necessary to add each new hook to every frontend driver
(and force other out-of-tree frontends to do the same).

But that isn't how things work now, and going down that particular
rabbit hole shouldn't be a requirement for this patch.  So IMO the
approach you're taking is fine.

The same goes for the new functions that you're exporting.  Ideally
they'd be (protected?) member functions of "driver", but the internal
callers are at points where the driver object is no longer to hand.
Again, exposing functions follows existing practice so IMO is fine.

> Here are the proposed patches and ChangeLogs and new files (gm2-v2.tar.gz)
> (after the patches):
>
> ./ChangeLog
>

Sorry for the changelog nits, but:

> 14-06-2019  Gaius Mulley   <gaius.mulley@southwales.ac.uk>

Should be: 2019-06-14
Should only be two spaces between your name and email address.
(Not that I care, I just work here :-))

>         * configure.ac (GM2_FOR_BUILD): Added.
>           (GM2_FOR_TARGET): Added.
>           Request build driver program gm2.

Lines should be indented to under the "*" rather than two spaces beyond.

>         * Makefile.def (GM2_FOR_TARGET): Added.
>           (GM2FLAGS_FOR_TARGET): Added.  Assign GM2,
>           GM2_FOR_BUILD, GM2_FOR_TARGET and GM2FLAGS.
>           Pass variables to make.  Add new language Modula-2
>           (m2).
>         * Makefile.tpl (GM2FLAGS): Added.  (GM2) Added.
>           (GM2_FOR_BUILD) Added.

Missing newline before "GM2".  Looks like some of the Makefile.def
entries belong in Makefile.tpl instead.  E.g. GM2_FOR_BUILD is only
mentioned in Makefile.tpl.

Reordering things slightly...

> @@ -1655,6 +1659,10 @@
>    { 0, 0 }
>  };
>  
> +/* front end registered spec functions */
> +static struct spec_function *lang_spec_functions = NULL;
> +static unsigned int lang_spec_functions_length = 0;
> +
>  static int processing_spec_function;
>  \f
>  /* Add appropriate libgcc specs to OBSTACK, taking into account
> [...]
> +/* Allow the front end to register a spec function.  */
> +
> +void
> +fe_add_spec_function (const char *name, const char *(*func) (int, const char **))
> +{
> +  const struct spec_function *f = lookup_spec_function (name);
> +  struct spec_function *fl;
> +  unsigned int i;
> +
> +  if (f != NULL)
> +    fatal_error (input_location, "spec function (%s) already registered", name);
> +
> +  if (lang_spec_functions == NULL)
> +    lang_spec_functions_length = 1;
> +
> +  lang_spec_functions_length++;
> +  fl = (struct spec_function *) xmalloc (sizeof (const struct spec_function)*lang_spec_functions_length);
> +  for (i=0; i<lang_spec_functions_length-2; i++)
> +    fl[i] = lang_spec_functions[i];
> +  free (lang_spec_functions);
> +  lang_spec_functions = fl;
> +
> +  lang_spec_functions[lang_spec_functions_length-2].name = name;
> +  lang_spec_functions[lang_spec_functions_length-2].func = func;
> +  lang_spec_functions[lang_spec_functions_length-1].name = NULL;
> +  lang_spec_functions[lang_spec_functions_length-1].func = NULL;
> +}
> +

This would be simpler if you make lang_spec_functions a vec<spec_function>.

> @@ -3725,6 +3733,70 @@
>    setenv ("SOURCE_DATE_EPOCH", source_date_epoch, 0);
>  }
>  
> +/* Save an option OPT with N_ARGS arguments in array ARGS, marking it
> +   as validated if VALIDATED.  */
> +
> +void
> +fe_save_switch (const char *opt, size_t n_args, const char *const *args,
> +		bool validated, bool known)
> +{
> +  save_switch (opt, n_args, args, validated, known);
> +}
> +
> +/* fe_B_prefix allow the front end to add its own -B option.  */
> +
> +void
> +fe_B_prefix (const char *arg)
> +{
> +  handle_OPT_B (arg);
> +}

It'd be simpler to expose save_switch and handle_OPT_B directly rather
than use wrappers.  The "fe_" prefix isn't used for other public functions.

> +
> +/* Handle the -B option by adding the prefix to exec, startfile and
> +   include search paths.  */
> +
> +static
> +void handle_OPT_B (const char *arg)

Formatting, should be:

static void
handle_OPT_B (const char *arg)

(although I guess no longer static after the above).

> +/* Mark a source file as compiled.  */
> +
> +void
> +fe_remove_infile (const char *name)
> +{
> +  int max = n_infiles + lang_specific_extra_outfiles;
> +  int i;
> +
> +  for (i = 0; i < max; i++)
> +    if (filename_cmp (name, infiles[i].name) == 0)
> +      infiles[i].compiled = true;
> +}

AFAICT this is only used here:

------------------------------------------------------------------------
/* no_objects return an empty string, but also remove all objects
   from the command line.  */

extern void fe_remove_infile (const char *);

static const char *
no_objects (int argc ATTRIBUTE_UNUSED, const char *argv[] ATTRIBUTE_UNUSED)
{
  object_list *o;

  for (o = head_objects; o != NULL; o = o->next)
    fe_remove_infile (o->name);

  return NULL;
}
------------------------------------------------------------------------

which looks unnecessarily quadratic.  Is there a simpler way to do this?

Either way, the name fe_remove_infile seems inconsistent with the
comment and with what the code actually does.

> --- gcc-versionno-orig/gcc/fortran/gfortranspec.c	2019-05-28 22:27:32.773238257 +0100
> +++ gcc-versionno/gcc/fortran/gfortranspec.c	2019-05-29 12:10:01.323261736 +0100
> @@ -448,3 +448,9 @@
>  
>  /* Number of extra output files that lang_specific_pre_link may generate.  */
>  int lang_specific_extra_outfiles = 0;	/* Not used for F77.  */
> +
> +/* lang_register_spec_functions.  Not used for F77.  */
> +void
> +lang_register_spec_functions (void)
> +{
> +}

Heh, the (existing) F77 comments look a bit of out of date :-)

Some comments about the tar file (wasn't sure whether this was part
of the submission yet, or whether you were just including it because
of Joseph's request in the earlier thread):

All in-tree configure files are now called configure.ac, so it would be
good if gm2 did the same.

------------------------------------------------------------------------
AC_INIT(gm2config.h.in, 1.9.1, gm2@nongnu.org)
------------------------------------------------------------------------

Would this stay the same after the merge, or would the Savannah version
of gm2 no longer be maintained?

------------------------------------------------------------------------
#ifndef MATH_LIBRARY_PROFILE
#define MATH_LIBRARY_PROFILE MATH_LIBRARY
#endif

#ifndef LIBSTDCXX
#define LIBSTDCXX "stdc++"
#endif

#ifndef LIBSTDCXX_PROFILE
#define LIBSTDCXX_PROFILE LIBSTDCXX
#endif

#ifndef LIBSTDCXX_STATIC
#define LIBSTDCXX_STATIC NULL
#endif
------------------------------------------------------------------------

These macros in gm2spec.c don't seem to be used.  "stdc++" in particular
is hard-coded further down.

------------------------------------------------------------------------
/* assert, a simple assertion function.  */

static void
assert (int b)
{
  if (!b)
    {
      printf ("assert failed in gm2spec.c");
      exit (1);
    }
}
------------------------------------------------------------------------

Better to use gcc_assert.

------------------------------------------------------------------------
#if defined(DEBUGGING)
static void
printOption (const char *desc, struct cl_decoded_option **in_decoded_options,
             int i)
{
------------------------------------------------------------------------

Realise it's only a debugging function, but should be print_option.

------------------------------------------------------------------------
/* The last entry in libraryName must be the longest string in the
list.  This is the -flibs=name.  */
static const char *libraryName[maxlib]
    = { "iso", "pim", "ulm", "min", "log", "cor" };
------------------------------------------------------------------------

Some comments like this aren't properly indented (but most are).
Mind having a quick scan and fix?  Realise it'll be a bit tedious, sorry.

Why does the last entry need to be the longest?  It only seems to be
used in a strcmp loop, and the list is separated by commas.

The corresponding option documentation says:

------------------------------------------------------------------------
flibs=
Modula-2 Joined
specify the library order, currently legal entries include: logitech, min, pim-coroutine, ulm, iso
------------------------------------------------------------------------

but it sounds like the actual values are: log, min, pim, ulm and iso,
is that right?  Would be worth clarifying the documentation line if so.

------------------------------------------------------------------------
/* get_archive_name, return the corresponding archive name given the library
   name.  */

static const char *
get_archive_name (const char *library)
{
  libs i;

  for (i = iso; i < maxlib; i = (libs) ((int)i + 1))
    if (strcmp (libraryName[i], library) == 0)
      return archiveName[i];
  return NULL;
}

/* build_archive, returns a string containing the a path to the
   archive defined by, libpath, s, and, dialectLib.  */

static char *
build_archive (const char *library)
{
  if (library != NULL)
    {
      const char *a = get_archive_name (library);
      if (a != NULL)
        {
          char *s = (char *)xmalloc (strlen (a) + 1);
          strcpy (s, a);
          return s;
        }
    }
  return NULL;
}
------------------------------------------------------------------------

The comment above build_archive seems to be a bit mangled.

It looks like this code silently ignores unrecognised -flibs= options.
Is that the intended behaviour?  I couldn't see where the valid values
were enforced.

------------------------------------------------------------------------
  else if (gm2_prefix != NULL && !seen_fmakeall0)

    /* no need to issue a warning if seen_fmakeall0 as the parent will
    have set COMPILER_PATH and LIBRARY_PATH because of GM2_ROOT and users
    should not be using -fmakeall0 as it is an internal option.  */
    fprintf (stderr,
             "warning it is not advisible to set " GM2_PREFIX_ENV
             " as well as either " LIBRARY_PATH_ENV " or COMPILER_PATH\n");
------------------------------------------------------------------------

This should be a proper warning, using the diagnostic machinery.

Posting the tar file definitely helped to show how these things are used.
But one thing I'm still not sure about is why gm2 needs it's own -B-related
code, and why for example it needs:

------------------------------------------------------------------------
/* find_executable_path, if argv0 references an executable filename
   then use this path.  */

static const char *
find_executable_path (const char *argv0)
{
  if (access (argv0, X_OK) == 0)
    {
      const char *n = strrchr (argv0, DIR_SEPARATOR);

      /* strip off the program name from argv0, but leave the DIR_SEPARATOR. */
      if (n != NULL)
        {
          char *copy = xstrdup (argv0);
          char *n = strrchr (copy, DIR_SEPARATOR);
          n[1] = (char)0;
          return copy;
        }
    }
  return NULL;
}
------------------------------------------------------------------------

GCC supports a lot of (maybe too many?) different installation layouts,
so it would be good to reuse the existing search code and self-relocation
code if at all possible.  That might mean exposing more functions to the
frontend driver, but IMO that'd still be preferable.

Thanks,
Richard