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

[Richard Biener <richard.guenther@gmail.com>]

On June 29, 2019 12:15:17 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>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.

I know we have multiple driver binaries but at the same time we support -x language. How will this continue to work with lang spec files when not integrating all FE drivers into one? 

Richard. 

>> 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