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

[Gaius Mulley]

Richard Sandiford <richard.sandiford@arm.com> writes:

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

Hi Richard,

many thanks for taking the time to review the patches and tarball.

I'm in the process of altering the ar and ranlib to gcc-ar/gcc-ranlib
and I'll test the -flto within gm2.  I think it should work - as gm2
utilises gcc to perform the final link and passes .o and .a files as
appropriate.  [Just finished testing and indeed it works - I need to
substitute ar and ranlib with their gcc-ar and gcc-ranlib counterparts].

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

sounds interesting - I guess this could result in a single driver binary
which could also make for a unified gcc?

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

yes all possible I imagine.

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

no problem - I'll adjust to the house convention.

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

ok

>>         * 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;
>>  
>>  /* 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>.

yes indeed!

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

sure

>> +
>> +/* 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.

yes, I'll improve this code.

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

ah yes thank you, well spotted - yes indeed this should be changed to
https://gcc.gnu.org/bugs (BUGURL). Sure I'll move everything to use .ac

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

sure

> ------------------------------------------------------------------------
> /* 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.

ok

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

yes

> ------------------------------------------------------------------------
> /* 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.

will do

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

will correct the documentation.  The libraries are log, min, pim, ulm,
iso and cor.  The last entry being the longest is no longer true - will
cull that comment.

> ------------------------------------------------------------------------
> /* 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.

yes true - I'll rewrite it.

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

yes it doesn't enforce them - I thought it would allow for 3rd party
libraries to be added alongside the gcc install.  But I guess it could
still check the directory names inside the install area if required.

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

ah yes sorry

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

this was to allow users to have multiple versions installed (a while
back).  I suspect it is time to cull this code (at least) and re-examine
the current installation support routines and use them instead - as you
mention.

> Thanks,
> Richard

thanks for the patch review,


regards,
Gaius