From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 2FB19385C426 for ; Tue, 26 May 2020 18:40:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2FB19385C426 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-370-7qxNg7h-N6GUHkCyqiUMyw-1; Tue, 26 May 2020 14:40:16 -0400 X-MC-Unique: 7qxNg7h-N6GUHkCyqiUMyw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AC7AD18FE860; Tue, 26 May 2020 18:40:15 +0000 (UTC) Received: from ovpn-112-12.phx2.redhat.com (ovpn-112-12.phx2.redhat.com [10.3.112.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id E39CD79C38; Tue, 26 May 2020 18:40:14 +0000 (UTC) Message-ID: <5de2a5202b50882612e1fe51d254f2b125f61716.camel@redhat.com> Subject: Re: [PATCH] Port libgccjit to Windows. From: David Malcolm To: Nicolas =?ISO-8859-1?Q?B=E9rtolo?= Cc: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Tue, 26 May 2020 14:40:14 -0400 In-Reply-To: References: <4b619179a08075bd2ee7f9e98aa2d5918191306d.camel@redhat.com> User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-15.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 May 2020 18:40:37 -0000 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?= > 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 . > + > +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 > +;. */ > + > +#include "config.h" > + > +#include > +#include > + > +#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(&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