Re: [PATCH] Port libgccjit to Windows.

[David Malcolm <dmalcolm@redhat.com>]

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)