On June 29, 2019 12:15:17 PM GMT+02:00, Richard Sandiford <richard.sandif...@arm.com> wrote: >Thanks for the patch and sorry for the slow reply. > >Gaius Mulley <gaius.mul...@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.mul...@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; >> > >> /* 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, g...@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