* [PATCH] Port libgccjit to Windows.
@ 2020-05-24 20:02 Nicolas Bértolo
2020-05-24 20:48 ` David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-05-24 20:02 UTC (permalink / raw)
To: jit, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 574 bytes --]
Hello gcc devs.
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.
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.
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.
Nicolas
[-- Attachment #2: 0001-Incomplete-port-of-libgccjit-to-Windows.patch --]
[-- Type: application/octet-stream, Size: 10762 bytes --]
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
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);
+ }
+#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);
+#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);
+ }
+#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);
+ }
+#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;
+
+ /* 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;
+ }
+
+ {
+ 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);
+ 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 ();
+#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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-24 20:02 [PATCH] Port libgccjit to Windows Nicolas Bértolo
@ 2020-05-24 20:48 ` David Malcolm
2020-05-25 19:48 ` Nicolas Bértolo
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-05-24 20:48 UTC (permalink / raw)
To: Nicolas Bértolo, jit, gcc-patches
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-24 20:48 ` David Malcolm
@ 2020-05-25 19:48 ` Nicolas Bértolo
2020-05-26 18:40 ` David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-05-25 19:48 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]
Hi Dave,
Thanks for your feedback.
> Do you have copyright assignment paperwork on file?
> https://gcc.gnu.org/contribute.html#legal
My paperwork is done.
> 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.
I guess the best solution would be to use libtool and Automake, but
that require major changes in the GCC build system. I came up with a
solution that uses an if statement to choose the way to build the
dynamic library.
> > 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 used case statement to check the value of $(target).
> What's the issue with fchmod on Windows? Does it not exist, or does it
> not work?
> Please add a comment explaining why.
fchmod does not exist in Windows.
> 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?
I created a new file "jit-w32.{h,c}" to store the definitions of
win_mkdtemp() and print_last_error().
> 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?
I added a new type "result::handle_t" that abstracts over this
difference.
> This seems like a very awkward way to get a temporary directory, but
> perhaps it's the best that can be done on Windows?
We could copy an implementation of mkdtemp () for Windows or write
something using a random number generator. I did it this way because
it was the fastest.
> Sorry if this seems excessively like nitpicking - thanks again for the
> patch.
No problem.
Here is a new version.
Nico.
[-- Attachment #2: 0001-Port-libgccjit-to-Windows.patch --]
[-- Type: application/octet-stream, Size: 20919 bytes --]
From a620c16adfadcebabd996a0b1d3761a582143ed7 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] Port libgccjit to Windows.
* configure: Regenerate.
* configure.ac: Don't require --enable-host-shared when building for Mingw.
* 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. Build libgccjit.dll and its import
library in Windows.
* gcc/jit/jit-w32.h: New file.
* gcc/jit/jit-w32.c (print_last_error): New function that prints the error
string corresponding to GetLastError().
(win_mkdtemp): Create a temporary directory using Windows APIs.
* gcc/jit/jit-playback.c: Do not chmod files in Windows. Use LoadLibrary,
FreeLibrary and GetProcAddress instead of libdl.
* gcc/jit/jit-result.{h,c}: Introduce result::handle_t to abstract over the
types used for dynamic library handles.
* gcc/jit/jit-tempdir.c: Do not use mkdtemp() in Windows, use win_mkdtemp().
---
configure | 32 +++++++------
configure.ac | 32 +++++++------
gcc/Makefile.in | 10 ++--
gcc/c/Make-lang.in | 2 +-
gcc/jit/Make-lang.in | 55 ++++++++++++++++++----
gcc/jit/jit-playback.c | 24 ++++++++--
gcc/jit/jit-result.c | 39 +++++++++++++---
gcc/jit/jit-result.h | 14 +++++-
gcc/jit/jit-tempdir.c | 10 ++++
gcc/jit/jit-w32.c | 102 +++++++++++++++++++++++++++++++++++++++++
gcc/jit/jit-w32.h | 30 ++++++++++++
11 files changed, 300 insertions(+), 50 deletions(-)
create mode 100644 gcc/jit/jit-w32.c
create mode 100644 gcc/jit/jit-w32.h
diff --git a/configure b/configure
index 3af6a530b..b7897446c 100755
--- a/configure
+++ b/configure
@@ -6489,9 +6489,13 @@ $as_echo "$as_me: WARNING: GNAT is required to build $language" >&2;}
esac
# Disable jit if -enable-host-shared not specified
- case ${add_this_lang}:${language}:${host_shared} in
- yes:jit:no)
- # PR jit/64780: explicitly specify --enable-host-shared
+ # but not if building for Mingw
+ case $target in
+ *mingw*) ;;
+ *)
+ case ${add_this_lang}:${language}:${host_shared} in
+ yes:jit:no)
+ # PR jit/64780: explicitly specify --enable-host-shared
as_fn_error $? "
Enabling language \"jit\" requires --enable-host-shared.
@@ -6502,17 +6506,19 @@ If you want to build both the jit and the regular compiler, it is often
best to do this via two separate configure/builds, in separate
directories, to avoid imposing the performance cost of
--enable-host-shared on the regular compiler." "$LINENO" 5
- ;;
- all:jit:no)
- { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: --enable-host-shared required to build $language" >&5
+ ;;
+ all:jit:no)
+ { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: --enable-host-shared required to build $language" >&5
$as_echo "$as_me: WARNING: --enable-host-shared required to build $language" >&2;}
- add_this_lang=unsupported
- ;;
- *:jit:no)
- # Silently disable.
- add_this_lang=unsupported
- ;;
- esac
+ add_this_lang=unsupported
+ ;;
+ *:jit:no)
+ # Silently disable.
+ add_this_lang=unsupported
+ ;;
+ esac
+ ;;
+ esac
# Disable a language that is unsupported by the target.
case "${add_this_lang}: $unsupported_languages " in
diff --git a/configure.ac b/configure.ac
index a67801371..edd963d3d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2079,9 +2079,13 @@ if test -d ${srcdir}/gcc; then
esac
# Disable jit if -enable-host-shared not specified
- case ${add_this_lang}:${language}:${host_shared} in
- yes:jit:no)
- # PR jit/64780: explicitly specify --enable-host-shared
+ # but not if building for Mingw
+ case $target in
+ *mingw*) ;;
+ *)
+ case ${add_this_lang}:${language}:${host_shared} in
+ yes:jit:no)
+ # PR jit/64780: explicitly specify --enable-host-shared
AC_MSG_ERROR([
Enabling language "jit" requires --enable-host-shared.
@@ -2092,16 +2096,18 @@ If you want to build both the jit and the regular compiler, it is often
best to do this via two separate configure/builds, in separate
directories, to avoid imposing the performance cost of
--enable-host-shared on the regular compiler.])
- ;;
- all:jit:no)
- AC_MSG_WARN([--enable-host-shared required to build $language])
- add_this_lang=unsupported
- ;;
- *:jit:no)
- # Silently disable.
- add_this_lang=unsupported
- ;;
- esac
+ ;;
+ all:jit:no)
+ AC_MSG_WARN([--enable-host-shared required to build $language])
+ add_this_lang=unsupported
+ ;;
+ *:jit:no)
+ # Silently disable.
+ add_this_lang=unsupported
+ ;;
+ esac
+ ;;
+ esac
# Disable a language that is unsupported by the target.
case "${add_this_lang}: $unsupported_languages " in
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 0fe2ba241..45851eca8 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,$(target)),,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..39d555650 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -40,10 +40,19 @@
# into the jit rule, but that needs a little bit of work
# to do the right thing within all.cross.
+ifneq (,$(findstring mingw,$(target)))
+LIBGCCJIT_FILENAME = libgccjit.dll
+
+jit: $(LIBGCCJIT_FILENAME) \
+ $(FULL_DRIVER_NAME)
+
+else
+
LIBGCCJIT_LINKER_NAME = libgccjit.so
LIBGCCJIT_VERSION_NUM = 0
LIBGCCJIT_MINOR_NUM = 0
LIBGCCJIT_RELEASE_NUM = 1
+
LIBGCCJIT_SONAME = $(LIBGCCJIT_LINKER_NAME).$(LIBGCCJIT_VERSION_NUM)
LIBGCCJIT_FILENAME = \
$(LIBGCCJIT_SONAME).$(LIBGCCJIT_MINOR_NUM).$(LIBGCCJIT_RELEASE_NUM)
@@ -68,6 +77,7 @@ jit: $(LIBGCCJIT_FILENAME) \
$(LIBGCCJIT_SYMLINK) \
$(LIBGCCJIT_LINKER_NAME_SYMLINK) \
$(FULL_DRIVER_NAME)
+endif
# Tell GNU make to ignore these if they exist.
.PHONY: jit
@@ -84,9 +94,27 @@ jit_OBJS = attribs.o \
jit/jit-spec.o \
gcc.o
+ifneq (,$(findstring mingw,$(target)))
+jit_OBJS += jit/jit-w32.o
+endif
+
# Use strict warnings for this front end.
jit-warn = $(STRICT_WARN)
+ifneq (,$(findstring mingw,$(target)))
+# We avoid using $(BACKEND) from Makefile.in in order to avoid pulling
+# in main.o
+$(LIBGCCJIT_FILENAME): $(jit_OBJS) \
+ libbackend.a libcommon-target.a libcommon.a \
+ $(CPPLIB) $(LIBDECNUMBER) \
+ $(LIBDEPS) $(srcdir)/jit/libgccjit.map \
+ $(EXTRA_GCC_OBJS)
+ +$(LLINKER) $(ALL_LINKERFLAGS) $(LDFLAGS) -o $@ -shared \
+ $(jit_OBJS) libbackend.a libcommon-target.a libcommon.a \
+ $(CPPLIB) $(LIBDECNUMBER) $(EXTRA_GCC_LIBS) $(LIBS) $(BACKENDLIBS) \
+ $(EXTRA_GCC_OBJS) \
+ -Wl,--out-implib,$(LIBGCCJIT_FILENAME).a
+else
# We avoid using $(BACKEND) from Makefile.in in order to avoid pulling
# in main.o
$(LIBGCCJIT_FILENAME): $(jit_OBJS) \
@@ -106,6 +134,7 @@ $(LIBGCCJIT_SONAME_SYMLINK): $(LIBGCCJIT_FILENAME)
$(LIBGCCJIT_LINKER_NAME_SYMLINK): $(LIBGCCJIT_SONAME_SYMLINK)
ln -sf $(LIBGCCJIT_SONAME_SYMLINK) $(LIBGCCJIT_LINKER_NAME_SYMLINK)
+endif
#\f
# Build hooks:
@@ -275,19 +304,29 @@ selftest-jit:
#\f
# Install hooks:
-jit.install-common: installdirs
+jit.install-headers:
+ $(INSTALL_DATA) $(srcdir)/jit/libgccjit.h \
+ $(DESTDIR)$(includedir)/libgccjit.h
+ $(INSTALL_DATA) $(srcdir)/jit/libgccjit++.h \
+ $(DESTDIR)$(includedir)/libgccjit++.h
+
+ifneq (,$(findstring mingw,$(target)))
+jit.install-common: installdirs jit.install-headers
+ $(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME).a \
+ $(DESTDIR)$(libdir)/$(LIBGCCJIT_FILENAME).a
$(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME) \
- $(DESTDIR)/$(libdir)/$(LIBGCCJIT_FILENAME)
+ $(DESTDIR)$(bindir)/$(LIBGCCJIT_FILENAME)
+else
+jit.install-common: installdirs jit.install-headers
+ $(INSTALL_PROGRAM) $(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)
- $(INSTALL_DATA) $(srcdir)/jit/libgccjit.h \
- $(DESTDIR)/$(includedir)/libgccjit.h
- $(INSTALL_DATA) $(srcdir)/jit/libgccjit++.h \
- $(DESTDIR)/$(includedir)/libgccjit++.h
+ $(DESTDIR)$(libdir)/$(LIBGCCJIT_LINKER_NAME_SYMLINK)
+endif
jit.install-man:
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index d2c8bb4c1..d1694e089 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 "jit-w32.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,10 @@ 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);
+ /* fchmod does not exist in Windows. */
+ #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 +2174,7 @@ playback::compile_to_file::copy_file (const char *src_path,
"error setting mode of %s: %s",
dst_path,
xstrerror (errno));
+ #endif
fclose (f_out);
}
@@ -2644,10 +2651,19 @@ dlopen_built_dso ()
{
JIT_LOG_SCOPE (get_logger ());
auto_timevar load_timevar (get_timer (), TV_LOAD);
- void *handle = NULL;
- const char *error = NULL;
+ result::handle_t handle = NULL;
result *result_obj = NULL;
+#ifdef _WIN32
+ /* Clear any existing error. */
+ SetLastError(0);
+
+ handle = LoadLibrary(m_tempdir->get_path_so_file ());
+ if (GetLastError() != 0) {
+ print_last_error();
+ }
+#else
+ const char *error = NULL;
/* Clear any existing error. */
dlerror ();
@@ -2656,6 +2672,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..605ae64ee 100644
--- a/gcc/jit/jit-result.c
+++ b/gcc/jit/jit-result.c
@@ -27,13 +27,17 @@ along with GCC; see the file COPYING3. If not see
#include "jit-result.h"
#include "jit-tempdir.h"
+#ifdef _WIN32
+#include "jit-w32.h"
+#endif
+
namespace gcc {
namespace jit {
/* Constructor for gcc::jit::result. */
result::
-result(logger *logger, void *dso_handle, tempdir *tempdir_) :
+result(logger *logger, handle_t dso_handle, tempdir *tempdir_) :
log_user (logger),
m_dso_handle (dso_handle),
m_tempdir (tempdir_)
@@ -49,8 +53,11 @@ result::~result()
{
JIT_LOG_SCOPE (get_logger ());
+#ifdef _WIN32
+ FreeLibrary(m_dso_handle);
+#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,17 @@ get_code (const char *funcname)
JIT_LOG_SCOPE (get_logger ());
void *code;
- const char *error;
+#ifdef _WIN32
+ /* Clear any existing error. */
+ SetLastError(0);
+
+ code = (void *)GetProcAddress(m_dso_handle, funcname);
+ if (GetLastError() != 0) {
+ print_last_error ();
+ }
+#else
+ const char *error;
/* Clear any existing error. */
dlerror ();
@@ -81,7 +97,8 @@ get_code (const char *funcname)
if ((error = dlerror()) != NULL) {
fprintf(stderr, "%s\n", error);
- }
+ }
+#endif
return code;
}
@@ -99,8 +116,17 @@ 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(m_dso_handle, name);
+ if (GetLastError() != 0) {
+ print_last_error ();
+ }
+#else
+ const char *error;
/* Clear any existing error. */
dlerror ();
@@ -108,7 +134,8 @@ get_global (const char *name)
if ((error = dlerror()) != NULL) {
fprintf(stderr, "%s\n", error);
- }
+ }
+#endif
return global;
}
diff --git a/gcc/jit/jit-result.h b/gcc/jit/jit-result.h
index 025860603..2ba87e58c 100644
--- a/gcc/jit/jit-result.h
+++ b/gcc/jit/jit-result.h
@@ -21,6 +21,10 @@ along with GCC; see the file COPYING3. If not see
#ifndef JIT_RESULT_H
#define JIT_RESULT_H
+#ifdef _WIN32
+#include <minwindef.h>
+#endif
+
namespace gcc {
namespace jit {
@@ -29,7 +33,13 @@ namespace jit {
class result : public log_user
{
public:
- result(logger *logger, void *dso_handle, tempdir *tempdir_);
+#ifdef _WIN32
+ using handle_t = HMODULE;
+#else
+ using handle_t = void *;
+#endif
+
+ result(logger *logger, handle_t dso_handle, tempdir *tempdir_);
virtual ~result();
@@ -40,7 +50,7 @@ public:
get_global (const char *name);
private:
- void *m_dso_handle;
+ handle_t m_dso_handle;
tempdir *m_tempdir;
};
diff --git a/gcc/jit/jit-tempdir.c b/gcc/jit/jit-tempdir.c
index 10c528faf..050d53409 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 "jit-w32.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,7 @@ make_tempdir_path_template ()
return result;
}
+#endif
/* The constructor for the jit::tempdir object.
The real work is done by the jit::tempdir::create method. */
@@ -87,6 +92,9 @@ gcc::jit::tempdir::create ()
{
JIT_LOG_SCOPE (get_logger ());
+#ifdef _WIN32
+ m_path_tempdir = win_mkdtemp ();
+#else
m_path_template = make_tempdir_path_template ();
if (!m_path_template)
return false;
@@ -97,6 +105,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);
diff --git a/gcc/jit/jit-w32.c b/gcc/jit/jit-w32.c
new file mode 100644
index 000000000..03da2d9f4
--- /dev/null
+++ b/gcc/jit/jit-w32.c
@@ -0,0 +1,102 @@
+/* Functions used by the Windows port of libgccjit.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ Contributed by Nicolas Bertolo <nicolasbertolo@gmail.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+
+#include <cstdio>
+#include <cstdint>
+
+#include "jit-w32.h"
+
+#include "libiberty.h"
+
+namespace gcc {
+namespace jit {
+void
+print_last_error (void)
+{
+ LPSTR psz{ nullptr };
+ DWORD dwErrorCode;
+ dwErrorCode = GetLastError();
+ const DWORD cchMsg = FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM
+ | FORMAT_MESSAGE_IGNORE_INSERTS
+ | FORMAT_MESSAGE_ALLOCATE_BUFFER
+ | FORMAT_MESSAGE_MAX_WIDTH_MASK,
+ NULL,
+ dwErrorCode,
+ MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+ reinterpret_cast<LPSTR>(&psz),
+ 0,
+ NULL);
+ if (cchMsg > 0)
+ {
+ fprintf (stderr, "%s\n", psz);
+ LocalFree (psz);
+ }
+ else
+ {
+ fprintf (stderr, "Failed to retrieve error message string for error %lu\n",
+ dwErrorCode);
+ }
+}
+
+char *
+win_mkdtemp (void)
+{
+ 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;
+
+ /* Generates a temporary file name. */
+ if (!GetTempFileNameA (lpTempPathBuffer, "gcc", 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;
+ }
+
+ {
+ char * result = XNEWVEC (char, MAX_PATH);
+ strcpy (result, szTempFileName);
+ return result;
+ }
+
+ error:
+ print_last_error ();
+ return NULL;
+}
+}
+}
diff --git a/gcc/jit/jit-w32.h b/gcc/jit/jit-w32.h
new file mode 100644
index 000000000..4ab50d259
--- /dev/null
+++ b/gcc/jit/jit-w32.h
@@ -0,0 +1,30 @@
+/* Functions used by the Windows port of libgccjit.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ Contributed by Nicolas Bertolo <nicolasbertolo@gmail.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+
+#include <windows.h>
+
+namespace gcc {
+namespace jit {
+extern void print_last_error (void);
+extern char * win_mkdtemp(void);
+}
+}
--
2.25.1.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-25 19:48 ` Nicolas Bértolo
@ 2020-05-26 18:40 ` David Malcolm
2020-05-28 1:27 ` Nicolas Bértolo
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-05-26 18:40 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: jit, gcc-patches
On Mon, 2020-05-25 at 16:48 -0300, Nicolas Bértolo wrote:
> Hi Dave,
>
> Thanks for your feedback.
>
> > Do you have copyright assignment paperwork on file?
> > https://gcc.gnu.org/contribute.html#legal
>
> My paperwork is done.
Thanks.
Do you have commit/push access to the gcc repository?
> > 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.
>
> I guess the best solution would be to use libtool and Automake, but
> that require major changes in the GCC build system. I came up with a
> solution that uses an if statement to choose the way to build the
> dynamic library.
That sounds reasonable.
> > > 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 used case statement to check the value of $(target).
BTW, why isn't it necessary to use --enable-host-shared in Windows?
Can we document that?
> > What's the issue with fchmod on Windows? Does it not exist, or does it
> > not work?
> > Please add a comment explaining why.
>
> fchmod does not exist in Windows.
Fair enough.
> > 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?
>
> I created a new file "jit-w32.{h,c}" to store the definitions of
> win_mkdtemp() and print_last_error().
Thanks. I like the idea of isolating the Windows-compatibility hooks
in one place.
> > 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?
>
> I added a new type "result::handle_t" that abstracts over this
> difference.
Thanks.
> > This seems like a very awkward way to get a temporary directory, but
> > perhaps it's the best that can be done on Windows?
>
> We could copy an implementation of mkdtemp () for Windows or write
> something using a random number generator. I did it this way because
> it was the fastest.
Fastest in terms of developer time, as opposed to for the computer,
right?
> > Sorry if this seems excessively like nitpicking - thanks again for the
> > patch.
>
> No problem.
On the subject of nitpicking, I find myself getting distracted by the
indentation in the patch; there seem to be a lot of mismatches.
What editor are you using, and does it have options to
(a) show visible whitespace, and
(b) to apply a formatting convention?
I use Emacs, and it takes care of this for me. I haven't used it, but
there's a contrib/clang-format file in the gcc source tree which
presumably describes GCC's coding conventions, if that helps for the
new code.
> Here is a new version.
Thanks. More review comments inline below...
> Nico.
>
> From a620c16adfadcebabd996a0b1d3761a582143ed7 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] Port libgccjit to Windows.
>
> * configure: Regenerate.
> * configure.ac: Don't require --enable-host-shared when building for Mingw.
> * 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. Build libgccjit.dll and its import
> library in Windows.
> * gcc/jit/jit-w32.h: New file.
> * gcc/jit/jit-w32.c (print_last_error): New function that prints the error
> string corresponding to GetLastError().
> (win_mkdtemp): Create a temporary directory using Windows APIs.
> * gcc/jit/jit-playback.c: Do not chmod files in Windows. Use LoadLibrary,
> FreeLibrary and GetProcAddress instead of libdl.
> * gcc/jit/jit-result.{h,c}: Introduce result::handle_t to abstract over the
> types used for dynamic library handles.
> * gcc/jit/jit-tempdir.c: Do not use mkdtemp() in Windows, use win_mkdtemp().
> ---
> configure | 32 +++++++------
> configure.ac | 32 +++++++------
> gcc/Makefile.in | 10 ++--
> gcc/c/Make-lang.in | 2 +-
> gcc/jit/Make-lang.in | 55 ++++++++++++++++++----
> gcc/jit/jit-playback.c | 24 ++++++++--
> gcc/jit/jit-result.c | 39 +++++++++++++---
> gcc/jit/jit-result.h | 14 +++++-
> gcc/jit/jit-tempdir.c | 10 ++++
> gcc/jit/jit-w32.c | 102 +++++++++++++++++++++++++++++++++++++++++
> gcc/jit/jit-w32.h | 30 ++++++++++++
> 11 files changed, 300 insertions(+), 50 deletions(-)
> create mode 100644 gcc/jit/jit-w32.c
> create mode 100644 gcc/jit/jit-w32.h
[...snip...]
> diff --git a/configure.ac b/configure.ac
> index a67801371..edd963d3d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2079,9 +2079,13 @@ if test -d ${srcdir}/gcc; then
> esac
>
> # Disable jit if -enable-host-shared not specified
> - case ${add_this_lang}:${language}:${host_shared} in
> - yes:jit:no)
> - # PR jit/64780: explicitly specify --enable-host-shared
> + # but not if building for Mingw
As mentioned above, why isn't it necessary? Can this be documented?
> + case $target in
> + *mingw*) ;;
> + *)
> + case ${add_this_lang}:${language}:${host_shared} in
> + yes:jit:no)
> + # PR jit/64780: explicitly specify --enable-host-shared
> AC_MSG_ERROR([
> Enabling language "jit" requires --enable-host-shared.
[...snip...]
> diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
> index 38ddfad28..39d555650 100644
> --- a/gcc/jit/Make-lang.in
> +++ b/gcc/jit/Make-lang.in
[...snip...]
> +ifneq (,$(findstring mingw,$(target)))
> +# We avoid using $(BACKEND) from Makefile.in in order to avoid pulling
> +# in main.o
> +$(LIBGCCJIT_FILENAME): $(jit_OBJS) \
> + libbackend.a libcommon-target.a libcommon.a \
> + $(CPPLIB) $(LIBDECNUMBER) \
> + $(LIBDEPS) $(srcdir)/jit/libgccjit.map \
> + $(EXTRA_GCC_OBJS)
> + +$(LLINKER) $(ALL_LINKERFLAGS) $(LDFLAGS) -o $@ -shared \
> + $(jit_OBJS) libbackend.a libcommon-target.a libcommon.a \
> + $(CPPLIB) $(LIBDECNUMBER) $(EXTRA_GCC_LIBS) $(LIBS) $(BACKENDLIBS) \
> + $(EXTRA_GCC_OBJS) \
> + -Wl,--out-implib,$(LIBGCCJIT_FILENAME).a
Please can you rework this so that there's a single rule for
$(LIBGCCJIT_FILENAME), to avoid duplication.
Looking at the existing rule, I think the only change the Windows
version has is that the final line is:
-Wl,--out-implib,$(LIBGCCJIT_FILENAME).a
whereas the existing code had these two lines:
$(LIBGCCJIT_VERSION_SCRIPT_OPTION) \
$(LIBGCCJIT_SONAME_OPTION)
So maybe it's best to introduce a new variable, LIBGCCJIT_EXTRA_OPTS,
or somesuch, conditionally defined in terms of the above two, up where
you define LIBGCCJIT_FILENAME etc.
> +else
> # We avoid using $(BACKEND) from Makefile.in in order to avoid pulling
> # in main.o
> $(LIBGCCJIT_FILENAME): $(jit_OBJS) \
> @@ -106,6 +134,7 @@ $(LIBGCCJIT_SONAME_SYMLINK): $(LIBGCCJIT_FILENAME)
>
> $(LIBGCCJIT_LINKER_NAME_SYMLINK): $(LIBGCCJIT_SONAME_SYMLINK)
> ln -sf $(LIBGCCJIT_SONAME_SYMLINK) $(LIBGCCJIT_LINKER_NAME_SYMLINK)
> +endif
>
> #
> # Build hooks:
> @@ -275,19 +304,29 @@ selftest-jit:
[...snip...]
> +ifneq (,$(findstring mingw,$(target)))
> +jit.install-common: installdirs jit.install-headers
> + $(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME).a \
> + $(DESTDIR)$(libdir)/$(LIBGCCJIT_FILENAME).a
Am I right in thinking that this installs the libgccjit.a file on Windows?
Why is this done?
[...snip...]
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index d2c8bb4c1..d1694e089 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 "jit-w32.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,10 @@ 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);
>
> + /* fchmod does not exist in Windows. */
> + #ifndef _WIN32
Something's up with the indentation here and in some other places.
The comment should be aligned with the "if" above, and preprocessor
directives such as #ifndef should be in the initial column.
> /* 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 +2174,7 @@ playback::compile_to_file::copy_file (const char *src_path,
> "error setting mode of %s: %s",
> dst_path,
> xstrerror (errno));
> + #endif
The #endif should be in the initial column.
> fclose (f_out);
> }
> @@ -2644,10 +2651,19 @@ dlopen_built_dso ()
> {
> JIT_LOG_SCOPE (get_logger ());
> auto_timevar load_timevar (get_timer (), TV_LOAD);
> - void *handle = NULL;
> - const char *error = NULL;
> + result::handle_t handle = NULL;
Another indentation issue; this should be aligned with the other decls
above.
[...snip...]
> diff --git a/gcc/jit/jit-result.h b/gcc/jit/jit-result.h
> index 025860603..2ba87e58c 100644
> --- a/gcc/jit/jit-result.h
> +++ b/gcc/jit/jit-result.h
[...snip...]
> @@ -29,7 +33,13 @@ namespace jit {
> class result : public log_user
> {
> public:
> - result(logger *logger, void *dso_handle, tempdir *tempdir_);
> +#ifdef _WIN32
> + using handle_t = HMODULE;
> +#else
> + using handle_t = void *;
> +#endif
IIRC the "using" syntax was introduced in C++11. Although we've been
thinking of moving from C++98 to C++11 for our implementation language,
I believe this hasn't happened yet, so please can you rewrite this to
use "typedef" instead.
[...snip...]
> diff --git a/gcc/jit/jit-w32.c b/gcc/jit/jit-w32.c
> new file mode 100644
> index 000000000..03da2d9f4
> --- /dev/null
> +++ b/gcc/jit/jit-w32.c
New C++ source files should have a .cc extension.
I hope that at some point we'll rename all the existing .c ones
accordingly.
> @@ -0,0 +1,102 @@
> +/* Functions used by the Windows port of libgccjit.
> + Copyright (C) 2020 Free Software Foundation, Inc.
> + Contributed by Nicolas Bertolo <nicolasbertolo@gmail.com>.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it
> +under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 3, or (at your option)
> +any later version.
> +
> +GCC is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3. If not see
> +<http://www.gnu.org/licenses/>;. */
> +
> +#include "config.h"
> +
> +#include <cstdio>
> +#include <cstdint>
> +
> +#include "jit-w32.h"
> +
> +#include "libiberty.h"
> +
> +namespace gcc {
> +namespace jit {
> +void
> +print_last_error (void)
> +{
> + LPSTR psz{ nullptr };
> + DWORD dwErrorCode;
> + dwErrorCode = GetLastError();
> + const DWORD cchMsg = FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM
> + | FORMAT_MESSAGE_IGNORE_INSERTS
> + | FORMAT_MESSAGE_ALLOCATE_BUFFER
> + | FORMAT_MESSAGE_MAX_WIDTH_MASK,
> + NULL,
> + dwErrorCode,
> + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> + reinterpret_cast<LPSTR>(&psz),
> + 0,
> + NULL);
> + if (cchMsg > 0)
> + {
> + fprintf (stderr, "%s\n", psz);
> + LocalFree (psz);
> + }
> + else
> + {
> + fprintf (stderr, "Failed to retrieve error message string for error %lu\n",
> + dwErrorCode);
> + }
> +}
More strange indentation above (e.g. the "if" doesn't line up with the
decls, the column of the "else", etc).
> +char *
> +win_mkdtemp (void)
> +{
> + 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;
> +
> + /* Generates a temporary file name. */
> + if (!GetTempFileNameA (lpTempPathBuffer, "gcc", 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!
Does this call generate a directory that's only accessible to the
current user?
Otherwise there could be a risk of a hostile user on the same machine
clobbering the contents and injecting code into this process.
> + /* 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;
> + }
> +
> + {
> + char * result = XNEWVEC (char, MAX_PATH);
> + strcpy (result, szTempFileName);
> + return result;
> + }
> +
> + error:
> + print_last_error ();
> + return NULL;
> +}
> +}
> +}
[...snip...]
Hope the above is constructive; thanks again.
Dave
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-26 18:40 ` David Malcolm
@ 2020-05-28 1:27 ` Nicolas Bértolo
2020-05-28 19:00 ` David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-05-28 1:27 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]
Hi,
> Do you have commit/push access to the gcc repository?
No I don't.
> BTW, why isn't it necessary to use --enable-host-shared in Windows?
> Can we document that?
That's because all code is position independent in Windows.
> On the subject of nitpicking, I find myself getting distracted by the
> indentation in the patch; there seem to be a lot of mismatches.
> What editor are you using, and does it have options to
> (a) show visible whitespace, and
> (b) to apply a formatting convention?
> I use Emacs, and it takes care of this for me. I haven't used it, but
> there's a contrib/clang-format file in the gcc source tree which
> presumably describes GCC's coding conventions, if that helps for the
> new code.
The problem seems to be that I was writing tabs but since I have set up my
editor to show them as 2 spaces I couldn't see what was wrong.
> Am I right in thinking that this installs the libgccjit.a file on Windows?
> Why is this done?
That is the file libgccjit.dll.a
It is the import library for gccjit. It is part of the way Windows handles
dynamic libraries.
> New C++ source files should have a .cc extension.
> I hope that at some point we'll rename all the existing .c ones
> accordingly.
I just couldn't get Make to generate jit-w32.o from jit-w32.cc.
It looks for jit-w32.c.
I had to leave it with the .c extension.
> Does this call generate a directory that's only accessible to the
> current user?
> Otherwise there could be a risk of a hostile user on the same machine
> clobbering the contents and injecting code into this process.
I changed the code to generate a directory than can only be accessed by the
current user.
I've attached a new version. It contains a rewrite of the code that creates
temporary directories.
Nico
[-- Attachment #2: 0001-Port-libgccjit-to-Windows.patch --]
[-- Type: application/octet-stream, Size: 26872 bytes --]
From f9f560132837b3454cce93d6098d05973f09ee8b 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] Port libgccjit to Windows.
* configure: Regenerate.
* configure.ac: Don't require --enable-host-shared when building for Mingw.
* 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. Build libgccjit.dll and its import
library in Windows.
* gcc/jit/jit-w32.h: New file.
* gcc/jit/jit-w32.c (print_last_error): New function that prints the error
string corresponding to GetLastError().
(get_TOKEN_USER_current_user): Helper function used for getting the SID
belonging to the current user.
(create_directory_for_current_user): Helper function to create a directory with
permissions such that only the current user can access it.
(win_mkdtemp): Create a temporary directory using Windows APIs.
* gcc/jit/jit-playback.c: Do not chmod files in Windows. Use LoadLibrary,
FreeLibrary and GetProcAddress instead of libdl.
* gcc/jit/jit-result.{h,c}: Introduce result::handle_t to abstract over the
types used for dynamic library handles.
* gcc/jit/jit-tempdir.c: Do not use mkdtemp() in Windows, use win_mkdtemp().
---
configure | 32 +++---
configure.ac | 33 +++---
gcc/Makefile.in | 10 +-
gcc/c/Make-lang.in | 2 +-
gcc/jit/Make-lang.in | 56 +++++++--
gcc/jit/config-lang.in | 2 +-
gcc/jit/jit-playback.c | 24 +++-
gcc/jit/jit-result.c | 35 +++++-
gcc/jit/jit-result.h | 14 ++-
gcc/jit/jit-tempdir.c | 10 ++
gcc/jit/jit-w32.c | 255 +++++++++++++++++++++++++++++++++++++++++
gcc/jit/jit-w32.h | 30 +++++
12 files changed, 452 insertions(+), 51 deletions(-)
create mode 100644 gcc/jit/jit-w32.c
create mode 100644 gcc/jit/jit-w32.h
diff --git a/configure b/configure
index 3af6a530b..b7897446c 100755
--- a/configure
+++ b/configure
@@ -6489,9 +6489,13 @@ $as_echo "$as_me: WARNING: GNAT is required to build $language" >&2;}
esac
# Disable jit if -enable-host-shared not specified
- case ${add_this_lang}:${language}:${host_shared} in
- yes:jit:no)
- # PR jit/64780: explicitly specify --enable-host-shared
+ # but not if building for Mingw
+ case $target in
+ *mingw*) ;;
+ *)
+ case ${add_this_lang}:${language}:${host_shared} in
+ yes:jit:no)
+ # PR jit/64780: explicitly specify --enable-host-shared
as_fn_error $? "
Enabling language \"jit\" requires --enable-host-shared.
@@ -6502,17 +6506,19 @@ If you want to build both the jit and the regular compiler, it is often
best to do this via two separate configure/builds, in separate
directories, to avoid imposing the performance cost of
--enable-host-shared on the regular compiler." "$LINENO" 5
- ;;
- all:jit:no)
- { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: --enable-host-shared required to build $language" >&5
+ ;;
+ all:jit:no)
+ { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: --enable-host-shared required to build $language" >&5
$as_echo "$as_me: WARNING: --enable-host-shared required to build $language" >&2;}
- add_this_lang=unsupported
- ;;
- *:jit:no)
- # Silently disable.
- add_this_lang=unsupported
- ;;
- esac
+ add_this_lang=unsupported
+ ;;
+ *:jit:no)
+ # Silently disable.
+ add_this_lang=unsupported
+ ;;
+ esac
+ ;;
+ esac
# Disable a language that is unsupported by the target.
case "${add_this_lang}: $unsupported_languages " in
diff --git a/configure.ac b/configure.ac
index a67801371..59bd92a3e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2079,9 +2079,14 @@ if test -d ${srcdir}/gcc; then
esac
# Disable jit if -enable-host-shared not specified
- case ${add_this_lang}:${language}:${host_shared} in
- yes:jit:no)
- # PR jit/64780: explicitly specify --enable-host-shared
+ # but not if building for Mingw. All code in Windows
+ # is position independent code (PIC).
+ case $target in
+ *mingw*) ;;
+ *)
+ case ${add_this_lang}:${language}:${host_shared} in
+ yes:jit:no)
+ # PR jit/64780: explicitly specify --enable-host-shared
AC_MSG_ERROR([
Enabling language "jit" requires --enable-host-shared.
@@ -2092,16 +2097,18 @@ If you want to build both the jit and the regular compiler, it is often
best to do this via two separate configure/builds, in separate
directories, to avoid imposing the performance cost of
--enable-host-shared on the regular compiler.])
- ;;
- all:jit:no)
- AC_MSG_WARN([--enable-host-shared required to build $language])
- add_this_lang=unsupported
- ;;
- *:jit:no)
- # Silently disable.
- add_this_lang=unsupported
- ;;
- esac
+ ;;
+ all:jit:no)
+ AC_MSG_WARN([--enable-host-shared required to build $language])
+ add_this_lang=unsupported
+ ;;
+ *:jit:no)
+ # Silently disable.
+ add_this_lang=unsupported
+ ;;
+ esac
+ ;;
+ esac
# Disable a language that is unsupported by the target.
case "${add_this_lang}: $unsupported_languages " in
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 0fe2ba241..45851eca8 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,$(target)),,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..9cb7814d6 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -40,10 +40,19 @@
# into the jit rule, but that needs a little bit of work
# to do the right thing within all.cross.
+ifneq (,$(findstring mingw,$(target)))
+LIBGCCJIT_FILENAME = libgccjit.dll
+
+jit: $(LIBGCCJIT_FILENAME) \
+ $(FULL_DRIVER_NAME)
+
+else
+
LIBGCCJIT_LINKER_NAME = libgccjit.so
LIBGCCJIT_VERSION_NUM = 0
LIBGCCJIT_MINOR_NUM = 0
LIBGCCJIT_RELEASE_NUM = 1
+
LIBGCCJIT_SONAME = $(LIBGCCJIT_LINKER_NAME).$(LIBGCCJIT_VERSION_NUM)
LIBGCCJIT_FILENAME = \
$(LIBGCCJIT_SONAME).$(LIBGCCJIT_MINOR_NUM).$(LIBGCCJIT_RELEASE_NUM)
@@ -68,6 +77,7 @@ jit: $(LIBGCCJIT_FILENAME) \
$(LIBGCCJIT_SYMLINK) \
$(LIBGCCJIT_LINKER_NAME_SYMLINK) \
$(FULL_DRIVER_NAME)
+endif
# Tell GNU make to ignore these if they exist.
.PHONY: jit
@@ -84,9 +94,21 @@ jit_OBJS = attribs.o \
jit/jit-spec.o \
gcc.o
+ifneq (,$(findstring mingw,$(target)))
+jit_OBJS += jit/jit-w32.o
+endif
+
# Use strict warnings for this front end.
jit-warn = $(STRICT_WARN)
+ifneq (,$(findstring mingw,$(target)))
+# Create import library libgccjit.dll.a
+LIBGCCJIT_EXTRA_OPTS = -Wl,--out-implib,$(LIBGCCJIT_FILENAME).a
+else
+LIBGCCJIT_EXTRA_OPTS = $(LIBGCCJIT_VERSION_SCRIPT_OPTION) \
+ $(LIBGCCJIT_SONAME_OPTION)
+endif
+
# We avoid using $(BACKEND) from Makefile.in in order to avoid pulling
# in main.o
$(LIBGCCJIT_FILENAME): $(jit_OBJS) \
@@ -98,14 +120,16 @@ $(LIBGCCJIT_FILENAME): $(jit_OBJS) \
$(jit_OBJS) libbackend.a libcommon-target.a libcommon.a \
$(CPPLIB) $(LIBDECNUMBER) $(EXTRA_GCC_LIBS) $(LIBS) $(BACKENDLIBS) \
$(EXTRA_GCC_OBJS) \
- $(LIBGCCJIT_VERSION_SCRIPT_OPTION) \
- $(LIBGCCJIT_SONAME_OPTION)
+ $(LIBGCCJIT_EXTRA_OPTS)
+# Create symlinks when not building for Windows
+ifeq (,$(findstring mingw,$(target)))
$(LIBGCCJIT_SONAME_SYMLINK): $(LIBGCCJIT_FILENAME)
ln -sf $(LIBGCCJIT_FILENAME) $(LIBGCCJIT_SONAME_SYMLINK)
$(LIBGCCJIT_LINKER_NAME_SYMLINK): $(LIBGCCJIT_SONAME_SYMLINK)
ln -sf $(LIBGCCJIT_SONAME_SYMLINK) $(LIBGCCJIT_LINKER_NAME_SYMLINK)
+endif
#\f
# Build hooks:
@@ -275,19 +299,31 @@ selftest-jit:
#\f
# Install hooks:
-jit.install-common: installdirs
+jit.install-headers:
+ $(INSTALL_DATA) $(srcdir)/jit/libgccjit.h \
+ $(DESTDIR)$(includedir)/libgccjit.h
+ $(INSTALL_DATA) $(srcdir)/jit/libgccjit++.h \
+ $(DESTDIR)$(includedir)/libgccjit++.h
+
+ifneq (,$(findstring mingw,$(target)))
+jit.install-common: installdirs jit.install-headers
+# Install import library
+ $(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME).a \
+ $(DESTDIR)$(libdir)/$(LIBGCCJIT_FILENAME).a
+# Install DLL file
$(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME) \
- $(DESTDIR)/$(libdir)/$(LIBGCCJIT_FILENAME)
+ $(DESTDIR)$(bindir)/$(LIBGCCJIT_FILENAME)
+else
+jit.install-common: installdirs jit.install-headers
+ $(INSTALL_PROGRAM) $(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)
- $(INSTALL_DATA) $(srcdir)/jit/libgccjit.h \
- $(DESTDIR)/$(includedir)/libgccjit.h
- $(INSTALL_DATA) $(srcdir)/jit/libgccjit++.h \
- $(DESTDIR)/$(includedir)/libgccjit++.h
+ $(DESTDIR)$(libdir)/$(LIBGCCJIT_LINKER_NAME_SYMLINK)
+endif
jit.install-man:
diff --git a/gcc/jit/config-lang.in b/gcc/jit/config-lang.in
index 89e3bfe0a..f8ef6042b 100644
--- a/gcc/jit/config-lang.in
+++ b/gcc/jit/config-lang.in
@@ -32,7 +32,7 @@ target_libs=""
gtfiles="\$(srcdir)/jit/dummy-frontend.c"
# The configuration requires --enable-host-shared
-# for jit to be supported.
+# for jit to be supported (only when not building for Mingw).
# Hence to get the jit, one must configure with:
# --enable-host-shared --enable-languages=jit
build_by_default="no"
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index d2c8bb4c1..0fddf04da 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 "jit-w32.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,10 @@ 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);
+ /* fchmod does not exist in Windows. */
+#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 +2174,7 @@ playback::compile_to_file::copy_file (const char *src_path,
"error setting mode of %s: %s",
dst_path,
xstrerror (errno));
+#endif
fclose (f_out);
}
@@ -2644,10 +2651,19 @@ dlopen_built_dso ()
{
JIT_LOG_SCOPE (get_logger ());
auto_timevar load_timevar (get_timer (), TV_LOAD);
- void *handle = NULL;
- const char *error = NULL;
+ result::handle handle = NULL;
result *result_obj = NULL;
+#ifdef _WIN32
+ /* Clear any existing error. */
+ SetLastError(0);
+
+ handle = LoadLibrary(m_tempdir->get_path_so_file ());
+ if (GetLastError() != 0) {
+ print_last_error();
+ }
+#else
+ const char *error = NULL;
/* Clear any existing error. */
dlerror ();
@@ -2656,6 +2672,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..633626412 100644
--- a/gcc/jit/jit-result.c
+++ b/gcc/jit/jit-result.c
@@ -27,13 +27,17 @@ along with GCC; see the file COPYING3. If not see
#include "jit-result.h"
#include "jit-tempdir.h"
+#ifdef _WIN32
+#include "jit-w32.h"
+#endif
+
namespace gcc {
namespace jit {
/* Constructor for gcc::jit::result. */
result::
-result(logger *logger, void *dso_handle, tempdir *tempdir_) :
+result(logger *logger, handle dso_handle, tempdir *tempdir_) :
log_user (logger),
m_dso_handle (dso_handle),
m_tempdir (tempdir_)
@@ -49,8 +53,11 @@ result::~result()
{
JIT_LOG_SCOPE (get_logger ());
+#ifdef _WIN32
+ FreeLibrary(m_dso_handle);
+#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,17 @@ get_code (const char *funcname)
JIT_LOG_SCOPE (get_logger ());
void *code;
- const char *error;
+#ifdef _WIN32
+ /* Clear any existing error. */
+ SetLastError(0);
+
+ code = (void *)GetProcAddress(m_dso_handle, funcname);
+ if (GetLastError() != 0) {
+ print_last_error ();
+ }
+#else
+ const char *error;
/* Clear any existing error. */
dlerror ();
@@ -82,6 +98,7 @@ get_code (const char *funcname)
if ((error = dlerror()) != NULL) {
fprintf(stderr, "%s\n", error);
}
+#endif
return code;
}
@@ -99,8 +116,17 @@ 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(m_dso_handle, name);
+ if (GetLastError() != 0) {
+ print_last_error ();
+ }
+#else
+ const char *error;
/* Clear any existing error. */
dlerror ();
@@ -109,6 +135,7 @@ get_global (const char *name)
if ((error = dlerror()) != NULL) {
fprintf(stderr, "%s\n", error);
}
+#endif
return global;
}
diff --git a/gcc/jit/jit-result.h b/gcc/jit/jit-result.h
index 025860603..d8e940aaf 100644
--- a/gcc/jit/jit-result.h
+++ b/gcc/jit/jit-result.h
@@ -21,6 +21,10 @@ along with GCC; see the file COPYING3. If not see
#ifndef JIT_RESULT_H
#define JIT_RESULT_H
+#ifdef _WIN32
+#include <minwindef.h>
+#endif
+
namespace gcc {
namespace jit {
@@ -29,7 +33,13 @@ namespace jit {
class result : public log_user
{
public:
- result(logger *logger, void *dso_handle, tempdir *tempdir_);
+#ifdef _WIN32
+ typedef HMODULE handle;
+#else
+ typedef void* handle;
+#endif
+
+ result(logger *logger, handle dso_handle, tempdir *tempdir_);
virtual ~result();
@@ -40,7 +50,7 @@ public:
get_global (const char *name);
private:
- void *m_dso_handle;
+ handle m_dso_handle;
tempdir *m_tempdir;
};
diff --git a/gcc/jit/jit-tempdir.c b/gcc/jit/jit-tempdir.c
index 10c528faf..050d53409 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 "jit-w32.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,7 @@ make_tempdir_path_template ()
return result;
}
+#endif
/* The constructor for the jit::tempdir object.
The real work is done by the jit::tempdir::create method. */
@@ -87,6 +92,9 @@ gcc::jit::tempdir::create ()
{
JIT_LOG_SCOPE (get_logger ());
+#ifdef _WIN32
+ m_path_tempdir = win_mkdtemp ();
+#else
m_path_template = make_tempdir_path_template ();
if (!m_path_template)
return false;
@@ -97,6 +105,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);
diff --git a/gcc/jit/jit-w32.c b/gcc/jit/jit-w32.c
new file mode 100644
index 000000000..99a6ed693
--- /dev/null
+++ b/gcc/jit/jit-w32.c
@@ -0,0 +1,255 @@
+/* Functions used by the Windows port of libgccjit.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ Contributed by Nicolas Bertolo <nicolasbertolo@gmail.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+
+/* Required for rand_s */
+#define _CRT_RAND_S
+
+#include <cstdio>
+#include <cstdint>
+
+#include "jit-w32.h"
+
+#include "libiberty.h"
+
+#include <accctrl.h>
+#include <aclapi.h>
+
+namespace gcc {
+namespace jit {
+void
+print_last_error (void)
+{
+ LPSTR psz = NULL;
+ DWORD dwErrorCode;
+ dwErrorCode = GetLastError();
+ const DWORD cchMsg = FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM
+ | FORMAT_MESSAGE_IGNORE_INSERTS
+ | FORMAT_MESSAGE_ALLOCATE_BUFFER
+ | FORMAT_MESSAGE_MAX_WIDTH_MASK,
+ NULL,
+ dwErrorCode,
+ MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+ reinterpret_cast<LPSTR>(&psz),
+ 0,
+ NULL);
+ if (cchMsg > 0)
+ {
+ fprintf (stderr, "%s\n", psz);
+ LocalFree (psz);
+ }
+ else
+ {
+ fprintf (stderr, "Failed to retrieve error message string for error %lu\n",
+ dwErrorCode);
+ }
+}
+
+/* Helper function used for getting the SID belonging to the current user. */
+static TOKEN_USER*
+get_TOKEN_USER_current_user ()
+{
+ TOKEN_USER *result = NULL;
+
+ HANDLE process_token = INVALID_HANDLE_VALUE;
+
+ DWORD token_user_info_len;
+ TOKEN_USER *token_user = NULL;
+
+ /* Get current process access token. */
+ if (!OpenProcessToken (GetCurrentProcess (), TOKEN_READ,
+ &process_token))
+ return NULL;
+
+ /* Get necessary buffer size. */
+ if (!GetTokenInformation(process_token, TokenUser, NULL, 0, &token_user_info_len)
+ && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+ goto cleanup;
+
+ token_user = (TOKEN_USER*) new char[token_user_info_len];
+
+ /* Get info about the user of the process */
+ if (!GetTokenInformation (process_token, TokenUser, token_user,
+ token_user_info_len, &token_user_info_len))
+ goto cleanup;
+
+ result = token_user;
+
+ cleanup:
+ if (process_token != INVALID_HANDLE_VALUE)
+ CloseHandle(process_token);
+
+ if (token_user != NULL && result == NULL)
+ delete[] (char*)token_user;
+
+ return result;
+}
+
+/* Helper function to create a directory with permissions such that only the
+ current user can access it. */
+static bool
+create_directory_for_current_user (const char * path)
+{
+ PACL pACL = NULL;
+ EXPLICIT_ACCESS ea;
+ SECURITY_ATTRIBUTES sa;
+ SECURITY_DESCRIPTOR SD;
+ DWORD dwRes;
+ bool result = true;
+ TOKEN_USER *token_user = NULL;
+
+ token_user = get_TOKEN_USER_current_user();
+ if (!token_user)
+ return false;
+
+ memset (&ea, 0, sizeof (EXPLICIT_ACCESS));
+ ea.grfAccessPermissions = GENERIC_ALL; /* Access to all. */
+ ea.grfAccessMode = SET_ACCESS; /* Set access and revoke everything else. */
+ /* This is necessary for the Windows Explorer GUI to show the correct tick
+ boxes in the "Security" tab. */
+ ea.grfInheritance = OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE;
+ ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
+ ea.Trustee.TrusteeType = TRUSTEE_IS_USER;
+ ea.Trustee.ptstrName = (char*) token_user->User.Sid;
+
+ /* Create a new ACL that contains the new ACEs. */
+ dwRes = SetEntriesInAcl(1, &ea, NULL, &pACL);
+ if (dwRes != ERROR_SUCCESS)
+ return false;
+
+ if (!InitializeSecurityDescriptor (&SD,
+ SECURITY_DESCRIPTOR_REVISION))
+ goto cleanup;
+
+ /* Add the ACL to the security descriptor. */
+ if (!SetSecurityDescriptorDacl (&SD,
+ TRUE, /* use pACL */
+ pACL,
+ FALSE)) /* not a default DACL */
+ goto cleanup;
+
+ /* Initialize a security attributes structure. */
+ sa.nLength = sizeof (SECURITY_ATTRIBUTES);
+ sa.lpSecurityDescriptor = &SD;
+ sa.bInheritHandle = FALSE;
+
+ /* Finally create the directory */
+ if (!CreateDirectoryA (path, &sa))
+ result = false;
+
+ cleanup:
+ if (pACL)
+ LocalFree (pACL);
+
+ if (token_user)
+ delete[] (char*)token_user;
+
+ return result;
+}
+
+
+char *
+win_mkdtemp (void)
+{
+ char lpTempPathBuffer[MAX_PATH];
+
+ /* 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))
+ {
+ print_last_error ();
+ return NULL;
+ }
+
+ /* Check that the directory actually exists. */
+ DWORD dwAttrib = GetFileAttributes (lpTempPathBuffer);
+ bool temp_path_exists = (dwAttrib != INVALID_FILE_ATTRIBUTES
+ && (dwAttrib & FILE_ATTRIBUTE_DIRECTORY));
+ if (!temp_path_exists)
+ {
+ fprintf (stderr, "Path returned by GetTempPath does not exist: %s\n",
+ lpTempPathBuffer);
+ }
+
+ /* Make sure there is enough space in the buffer for the prefix and random
+ number.*/
+ int temp_path_buffer_len = dwRetVal;
+ const int appended_len = strlen ("\\libgccjit-123456");
+ if (temp_path_buffer_len + appended_len + 1 >= MAX_PATH)
+ {
+ fprintf (stderr, "Temporary file path too long for generation of random"
+ " directories: %s", lpTempPathBuffer);
+ }
+
+ /* This is all the space we have in the buffer to store the random number and
+ prefix. */
+ int extraspace = MAX_PATH - temp_path_buffer_len - 1;
+
+ int tries;
+ const int max_tries = 1000;
+
+ for (tries = 0; tries < max_tries; ++tries)
+ {
+ /* Get a random number in [0; UINT_MAX]. */
+ unsigned int rand_num;
+ if (rand_s (&rand_num) != 0)
+ {
+ fprintf (stderr,
+ "Failed to create a random number using rand_s(): %s\n",
+ _strerror (NULL));
+ return NULL;
+ }
+
+ /* Create 6 digits random number. */
+ rand_num = ((double)rand_num / ((double) UINT_MAX + 1 ) * 1000000);
+
+ /* Copy the prefix and random number to the buffer. */
+ snprintf (&lpTempPathBuffer[temp_path_buffer_len], extraspace,
+ "\\libgccjit-%06u", rand_num);
+
+ if (create_directory_for_current_user (lpTempPathBuffer))
+ break; // success!
+
+ /* If we can't create the directory because we got unlucky and the
+ directory already exists retry, otherwise fail. */
+ if (GetLastError () != ERROR_ALREADY_EXISTS)
+ {
+ print_last_error ();
+ return NULL;
+ }
+ }
+
+ if (tries == max_tries)
+ {
+ fprintf (stderr, "Failed to create a random directory in %s\n",
+ lpTempPathBuffer);
+ return NULL;
+ }
+
+ {
+ int allocate_len = temp_path_buffer_len + appended_len + 1;
+ char * result = XNEWVEC (char, allocate_len);
+ strcpy (result, lpTempPathBuffer);
+ return result;
+ }
+}
+}
+}
diff --git a/gcc/jit/jit-w32.h b/gcc/jit/jit-w32.h
new file mode 100644
index 000000000..4ab50d259
--- /dev/null
+++ b/gcc/jit/jit-w32.h
@@ -0,0 +1,30 @@
+/* Functions used by the Windows port of libgccjit.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ Contributed by Nicolas Bertolo <nicolasbertolo@gmail.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+
+#include <windows.h>
+
+namespace gcc {
+namespace jit {
+extern void print_last_error (void);
+extern char * win_mkdtemp(void);
+}
+}
--
2.25.1.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-28 1:27 ` Nicolas Bértolo
@ 2020-05-28 19:00 ` David Malcolm
2020-05-28 19:51 ` Nicolas Bértolo
2020-05-29 14:48 ` NightStrike
0 siblings, 2 replies; 16+ messages in thread
From: David Malcolm @ 2020-05-28 19:00 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: jit, gcc-patches
On Wed, 2020-05-27 at 22:27 -0300, Nicolas Bértolo wrote:
> Hi,
>
> > Do you have commit/push access to the gcc repository?
>
> No I don't.
>
> > BTW, why isn't it necessary to use --enable-host-shared in Windows?
> > Can we document that?
>
> That's because all code is position independent in Windows.
>
> > On the subject of nitpicking, I find myself getting distracted by
> the
> > indentation in the patch; there seem to be a lot of mismatches.
>
> > What editor are you using, and does it have options to
> > (a) show visible whitespace, and
> > (b) to apply a formatting convention?
>
> > I use Emacs, and it takes care of this for me. I haven't used it,
> but
> > there's a contrib/clang-format file in the gcc source tree which
> > presumably describes GCC's coding conventions, if that helps for
> the
> > new code.
>
> The problem seems to be that I was writing tabs but since I have set
> up my
> editor to show them as 2 spaces I couldn't see what was wrong.
Thanks; the latest patch is much better.
> > Am I right in thinking that this installs the libgccjit.a file on
> Windows?
> > Why is this done?
>
> That is the file libgccjit.dll.a
>
> It is the import library for gccjit. It is part of the way Windows
> handles
> dynamic libraries.
Thanks.
> > New C++ source files should have a .cc extension.
> > I hope that at some point we'll rename all the existing .c ones
> > accordingly.
>
> I just couldn't get Make to generate jit-w32.o from jit-w32.cc.
> It looks for jit-w32.c.
>
> I had to leave it with the .c extension.
Fair enough.
> > Does this call generate a directory that's only accessible to the
> > current user?
> > Otherwise there could be a risk of a hostile user on the same
> machine
> > clobbering the contents and injecting code into this process.
>
> I changed the code to generate a directory than can only be accessed
> by the
> current user.
>
> I've attached a new version. It contains a rewrite of the code that
> creates
> temporary directories.
>
> Nico
I'm going to have to trust your Windows expertise here; the tempdir
code looks convoluted to me, but perhaps that's the only way to do it.
(Microsoft's docs for "SECURITY_ATTRIBUTES" suggest to me that if
lpSecurityDescriptor is NULL, then the directory gets a default
security descriptor, and that this may mean it's only readable by the
user represented by the access token of the process [1], which might
suggest a simplification - but I'm very hazy on how the security model
in Windows works)
I was able to successfully bootstrap and regression test with your
patch on x86_64-pc-linux-gnu. I also verified that the result of "make
install" was not affected for my configuration.
I've pushed your patch to master as
c83027f32d9cca84959c7d6a1e519a0129731501.
(I had to do a little fixup of the ChangeLog entries to get them to
work with the new hooks on our git repo)
Thanks again for the patch
Dave
[1] https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa379560(v=vs.85)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-28 19:00 ` David Malcolm
@ 2020-05-28 19:51 ` Nicolas Bértolo
2020-05-28 20:46 ` David Malcolm
2020-05-29 14:48 ` NightStrike
1 sibling, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-05-28 19:51 UTC (permalink / raw)
To: David Malcolm; +Cc: jit, gcc-patches
> I'm going to have to trust your Windows expertise here; the tempdir
> code looks convoluted to me, but perhaps that's the only way to do it.
> (Microsoft's docs for "SECURITY_ATTRIBUTES" suggest to me that if
> lpSecurityDescriptor is NULL, then the directory gets a default
> security descriptor, and that this may mean it's only readable by the
> user represented by the access token of the process [1], which might
> suggest a simplification - but I'm very hazy on how the security model
> in Windows works)
I tested this and it gives write access to the "Authenticated Users" group.
The
way I did it gives access only to the user that owns the libgccjit process.
I
have to admit that it is a lot of code and it is hard to understand unless
you
know the security model of Windows well. I don't know it well, I wrote this
keeping the documentation close and experimenting.
> I was able to successfully bootstrap and regression test with your
> patch on x86_64-pc-linux-gnu. I also verified that the result of "make
> install" was not affected for my configuration.
Great.
> I've pushed your patch to master as
> c83027f32d9cca84959c7d6a1e519a0129731501.
>
> Thanks again for the patch
> Dave
Thanks to you for all the good feedback.
Nico.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-28 19:51 ` Nicolas Bértolo
@ 2020-05-28 20:46 ` David Malcolm
2020-06-02 16:26 ` JonY
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2020-05-28 20:46 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: jit, gcc-patches
On Thu, 2020-05-28 at 16:51 -0300, Nicolas Bértolo wrote:
> > I'm going to have to trust your Windows expertise here; the tempdir
> > code looks convoluted to me, but perhaps that's the only way to do
> it.
> > (Microsoft's docs for "SECURITY_ATTRIBUTES" suggest to me that if
> > lpSecurityDescriptor is NULL, then the directory gets a default
> > security descriptor, and that this may mean it's only readable by
> the
> > user represented by the access token of the process [1], which
> might
> > suggest a simplification - but I'm very hazy on how the security
> model
> > in Windows works)
>
> I tested this and it gives write access to the "Authenticated Users"
> group.
Aha - sounds like that would be a problem. Thanks for clarifying.
> The
> way I did it gives access only to the user that owns the libgccjit
> process. I
> have to admit that it is a lot of code and it is hard to understand
> unless you
> know the security model of Windows well. I don't know it well, I
> wrote this
> keeping the documentation close and experimenting.
Thanks.
> > I was able to successfully bootstrap and regression test with your
> > patch on x86_64-pc-linux-gnu. I also verified that the result of
> "make
> > install" was not affected for my configuration.
>
> Great.
>
> > I've pushed your patch to master as
> > c83027f32d9cca84959c7d6a1e519a0129731501.
> >
> > Thanks again for the patch
> > Dave
>
> Thanks to you for all the good feedback.
>
> Nico.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-28 19:00 ` David Malcolm
2020-05-28 19:51 ` Nicolas Bértolo
@ 2020-05-29 14:48 ` NightStrike
1 sibling, 0 replies; 16+ messages in thread
From: NightStrike @ 2020-05-29 14:48 UTC (permalink / raw)
To: David Malcolm; +Cc: Nicolas Bértolo, jit, GCC Patches, JonY, Kai Tietz
On Thu, May 28, 2020, 4:25 PM David Malcolm via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:
> On Wed, 2020-05-27 at 22:27 -0300, Nicolas Bértolo wrote:
> > > New C++ source files should have a .cc extension.
> > > I hope that at some point we'll rename all the existing .c ones
> > > accordingly.
> >
> > I just couldn't get Make to generate jit-w32.o from jit-w32.cc.
> > It looks for jit-w32.c.
> >
> > I had to leave it with the .c extension.
>
> Fair enough.
>
That's not a good reason to leave it like this. You should get a make
expert to help here.
I was able to successfully bootstrap and regression test with your
>
patch on x86_64-pc-linux-gnu. I also verified that the result of "make
> install" was not affected for my configuration.
>
> I've pushed your patch to master as
> c83027f32d9cca84959c7d6a1e519a0129731501.
>
> (I had to do a little fixup of the ChangeLog entries to get them to
> work with the new hooks on our git repo)
>
> Thanks again for the patch
> Dave
>
> [1]
> https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa379560(v=vs.85)
I don't want to sound confrontational, but I don't think testing this on
linux and reviewing it by non windows experts is correct. At the very
least, a windows maintainer (Jon, Kai) should review it for correctness.
I've cc'd them here.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-05-28 20:46 ` David Malcolm
@ 2020-06-02 16:26 ` JonY
2020-06-07 16:03 ` Nicolas Bértolo
0 siblings, 1 reply; 16+ messages in thread
From: JonY @ 2020-06-02 16:26 UTC (permalink / raw)
To: David Malcolm, Nicolas Bértolo; +Cc: jit, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 2070 bytes --]
On 5/28/20 8:46 PM, David Malcolm via Gcc-patches wrote:
> On Thu, 2020-05-28 at 16:51 -0300, Nicolas Bértolo wrote:
>>> I'm going to have to trust your Windows expertise here; the tempdir
>>> code looks convoluted to me, but perhaps that's the only way to do
>> it.
>>> (Microsoft's docs for "SECURITY_ATTRIBUTES" suggest to me that if
>>> lpSecurityDescriptor is NULL, then the directory gets a default
>>> security descriptor, and that this may mean it's only readable by
>> the
>>> user represented by the access token of the process [1], which
>> might
>>> suggest a simplification - but I'm very hazy on how the security
>> model
>>> in Windows works)
>>
>> I tested this and it gives write access to the "Authenticated Users"
>> group.
>
> Aha - sounds like that would be a problem. Thanks for clarifying.
>
>> The
>> way I did it gives access only to the user that owns the libgccjit
>> process. I
>> have to admit that it is a lot of code and it is hard to understand
>> unless you
>> know the security model of Windows well. I don't know it well, I
>> wrote this
>> keeping the documentation close and experimenting.
>
> Thanks.
>
>>> I was able to successfully bootstrap and regression test with your
>>> patch on x86_64-pc-linux-gnu. I also verified that the result of
>> "make
>>> install" was not affected for my configuration.
>>
>> Great.
>>
>>> I've pushed your patch to master as
>>> c83027f32d9cca84959c7d6a1e519a0129731501.
>>>
>>> Thanks again for the patch
>>> Dave
>>
>> Thanks to you for all the good feedback.
>>
>> Nico.
>
Hello,
A bit of a late review, some minor points:
1. Using .so on Windows for DLLs is fine.
2. The DLL name on Windows should use LIBGCCJIT_SONAME rather than
LIBGCCJIT_LINKER_NAME, so applications would load libgccjit.so.0 instead
of libgccjit.so directly. The linker command output needs to be
LIBGCCJIT_SONAME.
3. Ideally I would prefer to .cc too, though I see other C++ files also
written as .c.
Resend in case reply to list did not work,
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-02 16:26 ` JonY
@ 2020-06-07 16:03 ` Nicolas Bértolo
2020-06-08 2:11 ` JonY
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-06-07 16:03 UTC (permalink / raw)
To: JonY; +Cc: David Malcolm, jit, gcc-patches
Hi,
Sorry for the super late reply.
> 1. Using .so on Windows for DLLs is fine.
I know, but using the standard suffix for the platform seems better, IMHO.
> 2. The DLL name on Windows should use LIBGCCJIT_SONAME rather than
> LIBGCCJIT_LINKER_NAME, so applications would load libgccjit.so.0 instead
> of libgccjit.so directly. The linker command output needs to be
> LIBGCCJIT_SONAME.
Do you think the library should be called libgccjit.so.0 instead of the more
Windows-like libgccjit.dll? That seems weird. Could you explain why?
Thanks, Nicolas.
El mar., 2 jun. 2020 a las 13:27, JonY (<10walls@gmail.com>) escribió:
>
> On 5/28/20 8:46 PM, David Malcolm via Gcc-patches wrote:
> > On Thu, 2020-05-28 at 16:51 -0300, Nicolas Bértolo wrote:
> >>> I'm going to have to trust your Windows expertise here; the tempdir
> >>> code looks convoluted to me, but perhaps that's the only way to do
> >> it.
> >>> (Microsoft's docs for "SECURITY_ATTRIBUTES" suggest to me that if
> >>> lpSecurityDescriptor is NULL, then the directory gets a default
> >>> security descriptor, and that this may mean it's only readable by
> >> the
> >>> user represented by the access token of the process [1], which
> >> might
> >>> suggest a simplification - but I'm very hazy on how the security
> >> model
> >>> in Windows works)
> >>
> >> I tested this and it gives write access to the "Authenticated Users"
> >> group.
> >
> > Aha - sounds like that would be a problem. Thanks for clarifying.
> >
> >> The
> >> way I did it gives access only to the user that owns the libgccjit
> >> process. I
> >> have to admit that it is a lot of code and it is hard to understand
> >> unless you
> >> know the security model of Windows well. I don't know it well, I
> >> wrote this
> >> keeping the documentation close and experimenting.
> >
> > Thanks.
> >
> >>> I was able to successfully bootstrap and regression test with your
> >>> patch on x86_64-pc-linux-gnu. I also verified that the result of
> >> "make
> >>> install" was not affected for my configuration.
> >>
> >> Great.
> >>
> >>> I've pushed your patch to master as
> >>> c83027f32d9cca84959c7d6a1e519a0129731501.
> >>>
> >>> Thanks again for the patch
> >>> Dave
> >>
> >> Thanks to you for all the good feedback.
> >>
> >> Nico.
> >
>
> Hello,
>
> A bit of a late review, some minor points:
>
> 1. Using .so on Windows for DLLs is fine.
> 2. The DLL name on Windows should use LIBGCCJIT_SONAME rather than
> LIBGCCJIT_LINKER_NAME, so applications would load libgccjit.so.0 instead
> of libgccjit.so directly. The linker command output needs to be
> LIBGCCJIT_SONAME.
> 3. Ideally I would prefer to .cc too, though I see other C++ files also
> written as .c.
>
> Resend in case reply to list did not work,
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-07 16:03 ` Nicolas Bértolo
@ 2020-06-08 2:11 ` JonY
2020-06-11 22:02 ` Nicolas Bértolo
0 siblings, 1 reply; 16+ messages in thread
From: JonY @ 2020-06-08 2:11 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: David Malcolm, jit, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 1173 bytes --]
On 6/7/20 4:03 PM, Nicolas Bértolo wrote:
> Hi,
>
> Sorry for the super late reply.
>
>> 1. Using .so on Windows for DLLs is fine.
>
> I know, but using the standard suffix for the platform seems better, IMHO.
>
It doesn't prevent applications from actually loading it.
>> 2. The DLL name on Windows should use LIBGCCJIT_SONAME rather than
>> LIBGCCJIT_LINKER_NAME, so applications would load libgccjit.so.0 instead
>> of libgccjit.so directly. The linker command output needs to be
>> LIBGCCJIT_SONAME.
>
> Do you think the library should be called libgccjit.so.0 instead of the more
> Windows-like libgccjit.dll? That seems weird. Could you explain why?
Ideally, libtool is used so we get libgccjit-0.dll, unfortunately it is
not. So the only way to ABI version the dll would be to use Unix style
soname to mark when an ABI has changed.
Applications loading libgccjit.so.0 should be recompiled if a new
incompatible ABI libgccjit.so.1 is introduced. If a recompile is not
possible, the older library can still be installed side by side.
Applications should still link against the file generated import library
for transparency.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-08 2:11 ` JonY
@ 2020-06-11 22:02 ` Nicolas Bértolo
2020-06-12 0:19 ` JonY
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Bértolo @ 2020-06-11 22:02 UTC (permalink / raw)
To: JonY; +Cc: David Malcolm, jit, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 504 bytes --]
Hi,
On 6/7/20 11:12 PM, JonY wrote:
> Ideally, libtool is used so we get libgccjit-0.dll, unfortunately it is
> not. So the only way to ABI version the dll would be to use Unix style
> soname to mark when an ABI has changed.
I tried generating the library as libgccjit-0.dll and naming its import library
libgccjit.dll.a. It worked. I understand you prefer the libgccjit-0.dll filename
from this comment about libtool. A patch implementing this change is attached.
Thanks for your feedback.
Nicolas.
[-- Attachment #2: 0001-Rename-libgccjit.dll-to-libgccjit-0.dll.patch --]
[-- Type: application/octet-stream, Size: 2185 bytes --]
From 62a68286338d4bd1db4d2c7075c85b6a53f5f198 Mon Sep 17 00:00:00 2001
From: Nicolas Bertolo <nicolasbertolo@gmail.com>
Date: Tue, 9 Jun 2020 11:37:51 -0300
Subject: [PATCH] Rename libgccjit.dll to libgccjit-0.dll.
* gcc/jit/Make-lang.in: Always define version, minor and release numbers. Create
the Windows shared library as libgccjit-$(LIBGCCJIT_VERSION_NUM).dll.
---
gcc/jit/Make-lang.in | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
index 9cb7814d6..3b543926c 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -40,8 +40,13 @@
# into the jit rule, but that needs a little bit of work
# to do the right thing within all.cross.
+LIBGCCJIT_VERSION_NUM = 0
+LIBGCCJIT_MINOR_NUM = 0
+LIBGCCJIT_RELEASE_NUM = 1
+
ifneq (,$(findstring mingw,$(target)))
-LIBGCCJIT_FILENAME = libgccjit.dll
+LIBGCCJIT_FILENAME = libgccjit-$(LIBGCCJIT_VERSION_NUM).dll
+LIBGCCJIT_IMPORT_LIB = libgccjit.dll.a
jit: $(LIBGCCJIT_FILENAME) \
$(FULL_DRIVER_NAME)
@@ -49,9 +54,6 @@ jit: $(LIBGCCJIT_FILENAME) \
else
LIBGCCJIT_LINKER_NAME = libgccjit.so
-LIBGCCJIT_VERSION_NUM = 0
-LIBGCCJIT_MINOR_NUM = 0
-LIBGCCJIT_RELEASE_NUM = 1
LIBGCCJIT_SONAME = $(LIBGCCJIT_LINKER_NAME).$(LIBGCCJIT_VERSION_NUM)
LIBGCCJIT_FILENAME = \
@@ -102,8 +104,8 @@ endif
jit-warn = $(STRICT_WARN)
ifneq (,$(findstring mingw,$(target)))
-# Create import library libgccjit.dll.a
-LIBGCCJIT_EXTRA_OPTS = -Wl,--out-implib,$(LIBGCCJIT_FILENAME).a
+# Create import library
+LIBGCCJIT_EXTRA_OPTS = -Wl,--out-implib,$(LIBGCCJIT_IMPORT_LIB)
else
LIBGCCJIT_EXTRA_OPTS = $(LIBGCCJIT_VERSION_SCRIPT_OPTION) \
$(LIBGCCJIT_SONAME_OPTION)
@@ -308,8 +310,8 @@ jit.install-headers:
ifneq (,$(findstring mingw,$(target)))
jit.install-common: installdirs jit.install-headers
# Install import library
- $(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME).a \
- $(DESTDIR)$(libdir)/$(LIBGCCJIT_FILENAME).a
+ $(INSTALL_PROGRAM) $(LIBGCCJIT_IMPORT_LIB) \
+ $(DESTDIR)$(libdir)/$(LIBGCCJIT_IMPORT_LIB)
# Install DLL file
$(INSTALL_PROGRAM) $(LIBGCCJIT_FILENAME) \
$(DESTDIR)$(bindir)/$(LIBGCCJIT_FILENAME)
--
2.25.1.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-11 22:02 ` Nicolas Bértolo
@ 2020-06-12 0:19 ` JonY
2020-06-16 0:12 ` JonY
0 siblings, 1 reply; 16+ messages in thread
From: JonY @ 2020-06-12 0:19 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: David Malcolm, jit, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 690 bytes --]
On 6/11/20 10:02 PM, Nicolas Bértolo wrote:
> Hi,
>
> On 6/7/20 11:12 PM, JonY wrote:
>> Ideally, libtool is used so we get libgccjit-0.dll, unfortunately it is
>> not. So the only way to ABI version the dll would be to use Unix style
>> soname to mark when an ABI has changed.
>
> I tried generating the library as libgccjit-0.dll and naming its import library
> libgccjit.dll.a. It worked. I understand you prefer the libgccjit-0.dll filename
> from this comment about libtool. A patch implementing this change is attached.
>
> Thanks for your feedback.
>
> Nicolas.
>
Thanks for the patch, it looks good to me. I will push this soon if no
one else objects.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-12 0:19 ` JonY
@ 2020-06-16 0:12 ` JonY
2020-06-16 0:15 ` Nicolas Bértolo
0 siblings, 1 reply; 16+ messages in thread
From: JonY @ 2020-06-16 0:12 UTC (permalink / raw)
To: Nicolas Bértolo; +Cc: David Malcolm, jit, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 781 bytes --]
On 6/12/20 12:19 AM, JonY wrote:
> On 6/11/20 10:02 PM, Nicolas Bértolo wrote:
>> Hi,
>>
>> On 6/7/20 11:12 PM, JonY wrote:
>>> Ideally, libtool is used so we get libgccjit-0.dll, unfortunately it is
>>> not. So the only way to ABI version the dll would be to use Unix style
>>> soname to mark when an ABI has changed.
>>
>> I tried generating the library as libgccjit-0.dll and naming its import library
>> libgccjit.dll.a. It worked. I understand you prefer the libgccjit-0.dll filename
>> from this comment about libtool. A patch implementing this change is attached.
>>
>> Thanks for your feedback.
>>
>> Nicolas.
>>
>
> Thanks for the patch, it looks good to me. I will push this soon if no
> one else objects.
>
>
Thanks, pushed to git master.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Port libgccjit to Windows.
2020-06-16 0:12 ` JonY
@ 2020-06-16 0:15 ` Nicolas Bértolo
0 siblings, 0 replies; 16+ messages in thread
From: Nicolas Bértolo @ 2020-06-16 0:15 UTC (permalink / raw)
To: JonY; +Cc: David Malcolm, jit, gcc-patches
> Thanks, pushed to git master.
Thanks, Nicolas.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-06-16 0:16 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-24 20:02 [PATCH] Port libgccjit to Windows Nicolas Bértolo
2020-05-24 20:48 ` David Malcolm
2020-05-25 19:48 ` Nicolas Bértolo
2020-05-26 18:40 ` David Malcolm
2020-05-28 1:27 ` Nicolas Bértolo
2020-05-28 19:00 ` David Malcolm
2020-05-28 19:51 ` Nicolas Bértolo
2020-05-28 20:46 ` David Malcolm
2020-06-02 16:26 ` JonY
2020-06-07 16:03 ` Nicolas Bértolo
2020-06-08 2:11 ` JonY
2020-06-11 22:02 ` Nicolas Bértolo
2020-06-12 0:19 ` JonY
2020-06-16 0:12 ` JonY
2020-06-16 0:15 ` Nicolas Bértolo
2020-05-29 14:48 ` NightStrike
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).