Re: [PATCH] Port libgccjit to Windows.

[David Malcolm <dmalcolm@redhat.com>]

On Sun, 2020-05-24 at 17:02 -0300, Nicolas Bértolo via Gcc-patches wrote:

> Hello gcc devs.

Hi Nicolas.

> I have ported libgccjit to Windows. I have tested it with the
> native-compilation branch of Emacs so I'm confident that it works well.

Excellent - thanks for doing this work.

Do you have copyright assignment paperwork on file?
https://gcc.gnu.org/contribute.html#legal

> The work is not finished though, I could use some help with these two
> points:
> 
> I have had to concede defeat to libtool and Automake. I could not get
> libgccjit
> to create a dll and put it in the correct directories. So for now we'll
> have to
> copy lib/libgccjit.so to bin/libgccjit.dll.

The autotools are not my strongest suit.

In a previous life I was a Windows developer, but I think it's been
about 18 years since I've done any coding on Windows, so I'm going to
have to trust your Windows expertise.

> It is not necessary to use --enable-host-shared in Windows (I tested
it),
> but I
> don't know the proper way to disable that check.

(I'm not sure here)

Various comments inline below...

> Nicolas
> 
> From 8644b979cf732e0b4d57c8281229fc3dcc9dc739 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo@gmail.com>
> Date: Fri, 22 May 2020 17:54:41 -0300
> Subject: [PATCH] Incomplete port of libgccjit to Windows.
> 
> * gcc/Makefile.in: don't look for libiberty in the "pic" subdirectory when
> building for Mingw. Add dependency on xgcc with the proper extension.
> * gcc/c/Make-lang.in: Remove extra slash.
> * gcc/jit/Make-lang.in: Remove extra slash.
> * gcc/jit/jit-playback.c: Do not chmod files in Windows. Use LoadLibrary,
> FreeLibrary and GetProcAddress instead of libdl.
> * gcc/jit/jit-tempdir.c: Do not use mkdtemp() in Windows. Get a filename with
> GetTempFileName.
> ---
>  gcc/Makefile.in        | 10 +++++---
>  gcc/c/Make-lang.in     |  2 +-
>  gcc/jit/Make-lang.in   | 10 ++++----
>  gcc/jit/jit-playback.c | 25 +++++++++++++++++--
>  gcc/jit/jit-result.c   | 46 ++++++++++++++++++++++++++++++----
>  gcc/jit/jit-tempdir.c  | 56 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 132 insertions(+), 17 deletions(-)
> 
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 0fe2ba241..e6dd9f59e 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1046,10 +1046,12 @@ ALL_LINKERFLAGS = $(ALL_CXXFLAGS)
>  
>  # Build and host support libraries.
>  
> -# Use the "pic" build of libiberty if --enable-host-shared.
> +# Use the "pic" build of libiberty if --enable-host-shared, unless we are
> +# building for mingw.
> +LIBIBERTY_PICDIR=$(if $(findstring mingw,$(build)),,pic)
>  ifeq ($(enable_host_shared),yes)
> -LIBIBERTY = ../libiberty/pic/libiberty.a
> -BUILD_LIBIBERTY = $(build_libobjdir)/libiberty/pic/libiberty.a
> +LIBIBERTY = ../libiberty/$(LIBIBERTY_PICDIR)/libiberty.a
> +BUILD_LIBIBERTY = $(build_libobjdir)/libiberty/$(LIBIBERTY_PICDIR)/libiberty.a
>  else
>  LIBIBERTY = ../libiberty/libiberty.a
>  BUILD_LIBIBERTY = $(build_libobjdir)/libiberty/libiberty.a
> @@ -1726,7 +1728,7 @@ MOSTLYCLEANFILES = insn-flags.h insn-config.h insn-codes.h \
>  # This symlink makes the full installation name of the driver be available
>  # from within the *build* directory, for use when running the JIT library
>  # from there (e.g. when running its testsuite).
> -$(FULL_DRIVER_NAME): ./xgcc
> +$(FULL_DRIVER_NAME): ./xgcc$(exeext)
>  	rm -f $@
>  	$(LN_S) $< $@
>  
> diff --git a/gcc/c/Make-lang.in b/gcc/c/Make-lang.in
> index 8944b9b9f..7efc7c2c3 100644
> --- a/gcc/c/Make-lang.in
> +++ b/gcc/c/Make-lang.in
> @@ -162,7 +162,7 @@ c.install-plugin: installdirs
>  # Install import library.
>  ifeq ($(plugin_implib),yes)
>  	$(mkinstalldirs) $(DESTDIR)$(plugin_resourcesdir)
> -	$(INSTALL_DATA) cc1$(exeext).a $(DESTDIR)/$(plugin_resourcesdir)/cc1$(exeext).a
> +	$(INSTALL_DATA) cc1$(exeext).a $(DESTDIR)$(plugin_resourcesdir)/cc1$(exeext).a
>  endif
>  
>  c.uninstall:
> diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
> index 38ddfad28..24f37c98b 100644
> --- a/gcc/jit/Make-lang.in
> +++ b/gcc/jit/Make-lang.in
> @@ -277,17 +277,17 @@ selftest-jit:
>  # Install hooks:
>  jit.install-common: installdirs
>  	$(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME) \
> -	  $(DESTDIR)/$(libdir)/$(LIBGCCJIT_FILENAME)
> +	  $(DESTDIR)$(libdir)/$(LIBGCCJIT_FILENAME)
>  	ln -sf \
>  	  $(LIBGCCJIT_FILENAME) \
> -	  $(DESTDIR)/$(libdir)/$(LIBGCCJIT_SONAME_SYMLINK)
> +	  $(DESTDIR)$(libdir)/$(LIBGCCJIT_SONAME_SYMLINK)
>  	ln -sf \
>  	  $(LIBGCCJIT_SONAME_SYMLINK)\
> -	  $(DESTDIR)/$(libdir)/$(LIBGCCJIT_LINKER_NAME_SYMLINK)
> +	  $(DESTDIR)$(libdir)/$(LIBGCCJIT_LINKER_NAME_SYMLINK)
>  	$(INSTALL_DATA) $(srcdir)/jit/libgccjit.h \
> -	  $(DESTDIR)/$(includedir)/libgccjit.h
> +	  $(DESTDIR)$(includedir)/libgccjit.h
>  	$(INSTALL_DATA) $(srcdir)/jit/libgccjit++.h \
> -	  $(DESTDIR)/$(includedir)/libgccjit++.h
> +	  $(DESTDIR)$(includedir)/libgccjit++.h
>  
>  jit.install-man:
>  
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index d2c8bb4c1..24f965244 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -47,6 +47,10 @@ along with GCC; see the file COPYING3.  If not see
>  #include "jit-builtins.h"
>  #include "jit-tempdir.h"
>  
> +#ifdef _WIN32
> +#include <windows.h>
> +#endif
> +
>  /* Compare with gcc/c-family/c-common.h: DECL_C_BIT_FIELD,
>     SET_DECL_C_BIT_FIELD.
>     These are redefined here to avoid depending from the C frontend.  */
> @@ -2159,8 +2163,9 @@ playback::compile_to_file::copy_file (const char *src_path,
>  
>    gcc_assert (total_sz_in == total_sz_out);
>    if (get_logger ())
> -    get_logger ()->log ("total bytes copied: %ld", total_sz_out);
> +    get_logger ()->log ("total bytes copied: %zu", total_sz_out);
>  
> +	#ifndef _WIN32
>    /* Set the permissions of the copy to those of the original file,
>       in particular the "executable" bits.  */
>    if (fchmod (fileno (f_out), stat_buf.st_mode) == -1)
> @@ -2168,6 +2173,7 @@ playback::compile_to_file::copy_file (const char *src_path,
>  	       "error setting mode of %s: %s",
>  	       dst_path,
>  	       xstrerror (errno));
> +	#endif

What's the issue with fchmod on Windows?  Does it not exist, or does it
not work?
Please add a comment explaining why.

>    fclose (f_out);
>  }
> @@ -2645,9 +2651,22 @@ dlopen_built_dso ()
>    JIT_LOG_SCOPE (get_logger ());
>    auto_timevar load_timevar (get_timer (), TV_LOAD);
>    void *handle = NULL;
> -  const char *error = NULL;
>    result *result_obj = NULL;
>  
> +#ifdef _WIN32
> +  /* Clear any existing error.  */
> +  SetLastError(0);
> +
> +  handle = LoadLibrary(m_tempdir->get_path_so_file ());
> +  if (GetLastError() != 0)  {
> +		char buf[256];
> +		FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +									 NULL, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +									 buf, sizeof(buf), NULL);
> +    fprintf(stderr, "%s\n", buf);
> +  }

Please can you fix the indentation here to stay within 80 columns.
It looks like there's very similar (identical?) code to this below in
several places.
Can it be put in a subroutine to avoid repetition?

> +#else
> +  const char *error = NULL;
>    /* Clear any existing error.  */
>    dlerror ();
>  
> @@ -2656,6 +2675,8 @@ dlopen_built_dso ()
>    if ((error = dlerror()) != NULL)  {
>      add_error (NULL, "%s", error);
>    }
> +#endif
> +
>    if (handle)
>      {
>        /* We've successfully dlopened the result; create a
> diff --git a/gcc/jit/jit-result.c b/gcc/jit/jit-result.c
> index c10e5a13c..6c87b35c7 100644
> --- a/gcc/jit/jit-result.c
> +++ b/gcc/jit/jit-result.c
> @@ -22,6 +22,10 @@ along with GCC; see the file COPYING3.  If not see
>  #include "system.h"
>  #include "coretypes.h"
>  
> +#ifdef _WIN32
> +#include <windows.h>
> +#endif
> +
>  #include "jit-common.h"
>  #include "jit-logging.h"
>  #include "jit-result.h"
> @@ -49,8 +53,11 @@ result::~result()
>  {
>    JIT_LOG_SCOPE (get_logger ());
>  
> +#ifdef _WIN32
> +  FreeLibrary((HMODULE)m_dso_handle);

Is there a cast to HMODULE everywhere that m_dso_handle is used?
If so, does it make the code cleaner if the field becomes an HMODULE on
Windows?

> +#else
>    dlclose (m_dso_handle);
> -
> +#endif
>    /* Responsibility for cleaning up the tempdir (including "fake.so" within
>       the filesystem) might have been handed to us by the playback::context,
>       so that the cleanup can be delayed (see PR jit/64206).
> @@ -72,8 +79,22 @@ get_code (const char *funcname)
>    JIT_LOG_SCOPE (get_logger ());
>  
>    void *code;
> -  const char *error;
>  
> +#ifdef _WIN32
> +  DWORD error;
> +  /* Clear any existing error.  */
> +  SetLastError(0);
> +
> +  code = (void *)GetProcAddress((HMODULE)m_dso_handle, funcname);
> +  if ((error = GetLastError()) != 0)  {
> +		char buf[256];
> +		FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +									 NULL, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +									 buf, sizeof(buf), NULL);
> +    fprintf(stderr, "%s\n", buf);
> +  }

Here's that repeated code I mentioned earlier.  

> +#else
> +  const char *error;
>    /* Clear any existing error.  */
>    dlerror ();
>  
> @@ -81,7 +102,8 @@ get_code (const char *funcname)
>  
>    if ((error = dlerror()) != NULL)  {
>      fprintf(stderr, "%s\n", error);
> -  }
> +	}
> +#endif
>  
>    return code;
>  }
> @@ -99,8 +121,21 @@ get_global (const char *name)
>    JIT_LOG_SCOPE (get_logger ());
>  
>    void *global;
> -  const char *error;
>  
> +#ifdef _WIN32
> +  /* Clear any existing error.  */
> +  SetLastError(0);
> +
> +  global = (void *)GetProcAddress((HMODULE)m_dso_handle, name);
> +  if (GetLastError() != 0)  {
> +		char buf[256];
> +		FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +									 NULL, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +									 buf, sizeof(buf), NULL);
> +    fprintf(stderr, "%s\n", buf);

Repetition again; can this be a subroutine, please.

> +  }
> +#else
> +  const char *error;
>    /* Clear any existing error.  */
>    dlerror ();
>  
> @@ -108,7 +143,8 @@ get_global (const char *name)
>  
>    if ((error = dlerror()) != NULL)  {
>      fprintf(stderr, "%s\n", error);
> -  }
> +	}
> +#endif
>  
>    return global;
>  }
> diff --git a/gcc/jit/jit-tempdir.c b/gcc/jit/jit-tempdir.c
> index 10c528faf..37fc3e113 100644
> --- a/gcc/jit/jit-tempdir.c
> +++ b/gcc/jit/jit-tempdir.c
> @@ -24,7 +24,11 @@ along with GCC; see the file COPYING3.  If not see
>  
>  #include "jit-tempdir.h"
>  
> +#ifdef _WIN32
> +#include "windows.h"
> +#endif
>  
> +#ifndef _WIN32
>  /* Construct a tempdir path template suitable for use by mkdtemp
>     e.g. "/tmp/libgccjit-XXXXXX", but respecting the rules in
>     libiberty's choose_tempdir rather than hardcoding "/tmp/".
> @@ -62,6 +66,53 @@ make_tempdir_path_template ()
>  
>    return result;
>  }
> +#else
> +static char *
> +win_mkdtemp()
> +{
> +	char szTempFileName[MAX_PATH];
> +	char lpTempPathBuffer[MAX_PATH];
> +
> +	while (true)
> +		{
> +			/* Gets the temp path env string (no guarantee it's a valid path). */
> +			DWORD dwRetVal = GetTempPath (MAX_PATH, lpTempPathBuffer);
> +			if (dwRetVal > MAX_PATH || (dwRetVal == 0))
> +				goto error;
> +

Something seems up with the indentation here; it seems indented too much.

> +			/* Generates a temporary file name. */
> +			if (!GetTempFileNameA (lpTempPathBuffer, "gccjit", 0, szTempFileName))
> +				goto error;
> +
> +			/* GetTempFileNameA actually creates a file, we are need a directory, so
> +				 remove it. */
> +			if (!DeleteFileA (szTempFileName))
> +				goto error;
> +
> +			if (CreateDirectoryA (szTempFileName, NULL))
> +				break; // success!
> +
> +			/* If we can't create the directory because we got unlucky and another
> +				 process created a file retry, otherwise fail. */
> +			if (GetLastError () != ERROR_ALREADY_EXISTS)
> +				goto error;
> +		}

This seems like a very awkward way to get a temporary directory, but
perhaps it's the best that can be done on Windows?

> +
> +	{
> +		char * result = XNEWVEC (char, MAX_PATH);
> +		strcpy (result, szTempFileName);
> +		return result;
> +	}
> +
> + error:
> +	/* Reuse szTempFileName for the error message. */
> +	FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +								 NULL, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +								 szTempFileName, sizeof(szTempFileName), NULL);

Again, can this be a subroutine, please.

> +	fprintf(stderr, "%s\n", szTempFileName);
> +	return NULL;
> +}
> +#endif
>  
>  /* The constructor for the jit::tempdir object.
>     The real work is done by the jit::tempdir::create method.  */
> @@ -87,6 +138,9 @@ gcc::jit::tempdir::create ()
>  {
>    JIT_LOG_SCOPE (get_logger ());
>  
> +#ifdef _WIN32
> +	m_path_tempdir = win_mkdtemp ();

(Indentation nit)

> +#else
>    m_path_template = make_tempdir_path_template ();
>    if (!m_path_template)
>      return false;
> @@ -97,6 +151,8 @@ gcc::jit::tempdir::create ()
>       is unique.  Hence no other (non-root) users should have access to
>       the paths within it.  */
>    m_path_tempdir = mkdtemp (m_path_template);
> +#endif
> +
>    if (!m_path_tempdir)
>      return false;
>    log ("m_path_tempdir: %s", m_path_tempdir);
> -- 
> 2.25.1.windows.1

Sorry if this seems excessively like nitpicking - thanks again for the
patch.

Dave