From: David Malcolm <dmalcolm@redhat.com>
To: "Nicolas Bértolo" <nicolasbertolo@gmail.com>
Cc: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Port libgccjit to Windows.
Date: Tue, 26 May 2020 14:40:14 -0400 [thread overview]
Message-ID: <5de2a5202b50882612e1fe51d254f2b125f61716.camel@redhat.com> (raw)
In-Reply-To: <CAFnS-O=uLi0UrxmFC=xvVgZ_SWP8naH3fhuDpR5Qmo9oR93Axw@mail.gmail.com>
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
next prev parent reply other threads:[~2020-05-26 18:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-24 20:02 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5de2a5202b50882612e1fe51d254f2b125f61716.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
--cc=nicolasbertolo@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).