[MLton] A few patches we had to apply to port from version 20030716 to 20051202 and for the MinGW port

[Wesley W. Terpstra]

I don't claim to have gone over the patches exhaustively yet (no  
windows access). However, I do have a few comments on what I've seen.

As no one else has said it yet: thanks for the patches! I'll add the  
disclaimer that I'm not one of the core MLton developers, but I've  
touched most of the code your patches affect, so I feel I should add  
my two cents before they're committed.

On Mar 28, 2007, at 4:34 PM, Nicolas Bertolotti wrote:
> - bug-fix-cygwin-1_3_17.patch :
>
>  We want our product to run on old versions of Cygwin. As a  
> consequence, we use a cross-compiler which produces code for Cygwin  
> 1.3.17. The current version of MLton is not compatible with it for  
> several reasons and this is why we had to patch it.
>
> The patch consists in :
>
> - providing 2 header files which are required by MLton but are not  
> provided with Cygwin 1.3.17 (inttypes.h and stdint.h)
>
> - implementing the function nanosleep (same implementation than the  
> one used for the MinGW target) which is not available on Cygwin 1.3.17
>
> - disabling the use of the function “fpclassify” which is broken on  
> Cygwin 1.3.17 and causes the expression “real(~1)” to be evaluated  
> as “nan”
>
> - handling the fact that the Cygwin 1.3.17 headers contain some  
> definitions related to IPv6 (sockaddr_in6 …) but does not define  
> others (sockaddr_storage)
>
> - defining MSG_CTRUNC and MSG_TRUNC which are not defined in the  
> Cygwin 1.3.17 headers
I question whether these should be applied. It seems reasonable to me  
to simply require that users use the newer cygwin. If the newest  
cygwin had these problems, then it might be another story. However,  
over time the old cygwin versions will vanish, so these patches will  
become cruft in the MLton source tree. Worse, the headers are not  
under the MLton copyright, nor can they be made so as none of us  
wrote them. It might cause problems to include them in the MLton  
distribution.

Perhaps we could instead offer this patch as a separate download?
> - bug-fix-solaris7.patch
>
> This patch is needed to build the MLton runtime when the gcc  
> compiler is designed to produce code for Solaris 7 because the  
> headers do not provide the definition for MAP_ANON IPv6.
Does this MAP_ANON value actually work? If so, is it declared in  
another header? If not, why?

Also, I don't think it's a good idea to use 'char dummy' as the  
contents of sockaddr_in6. A real IPv6 structure is much larger,  
leading to a potential overrun. Also, it is guaranteed that sizeof 
(sockaddr_storage) >= sizeof(sockaddr_*). Is sockaddr_in really the  
largest? sockaddr_in6 is usually bigger. Are these structures missing  
from headers or missing from the system's support?
>  - bug-fix-spawn-with-fork.patch
>
>  This patch consists in adding a call to “exit” when the “exec”  
> call fails after a successful “fork” in the “spawn” implementation.  
> If we don’t do so, the current process will be duplicated in case  
> the “exec” fails.
Oops.
>  - bug-fix-fullpath-windows.patch
>
>  This patch consists in providing an implementation of fullPath()  
> which works on Windows (Cygwin and MinGW). The original  
> implementation only works on Unix because it does not handle the  
> volume part of a path.
I've only read the code, not run it, but it seems reasonable.

What's this part about?
+                          (* Fix bug in isLink when NFS mounted  
problem *)
+                           if ((not isMinGW) andalso (isLink arc))  
handle _ =>
+                              raise PosixError.SysErr("System  
problem in path " ^ p, NONE)

isLink can raise an exception when it's on NFS? Should we pass this  
exception through?
>  - bug-fix-nethostdb-mingw.patch
>
>  This patch fixes the NetHostDB functions getByAddress(), getByName 
> () and getHostName()  which fail to work properly if the WinSock2  
> DLL has not been initialized before we call them.
I see that other parts of the basis are doing this as well.  
Therefore, I see no harm in putting these calls into those methods.

However, on another note, shouldn't we be calling this ONCE from the  
main method? I seem to recall it used to work this way? What happened?
> - bug-fix-rename-mingw.patch
>
>  This patch fixes the rename() function whose behavior is not the  
> same on MinGW than Unix (with MinGW, rename fails when the  
> destination file exists whereas the Unix specification is to  
> replace the existing file).
Sounds good.
> - bug-fix-times-mingw.patch
>
> This patch is designed to avoid raising an exception during the  
> initialization of the basis library on MinGW when the code contains  
> calls to the “times” function which are not detected as dead code  
> during compilation. Then, if the “times” function is called, the  
> exception will be raised from the location of the call which is  
> easier to understand from a user’s point of view.
No idea about this.
>  - bug-fix-closesocket-mingw.patch
>
>  This patch fixes the Socket.close() function on MinGW. On Windows,  
> a socket should be closed using the “closesocket” function rather  
> than the “close” function.
I guess it now becomes evident no one has used networking under  
MinGW. :->
> - evol-getpid-mingw.patch
>
> This patch enables the getpid() function which exists on Windows
I'm a bit concerned. We also return a 'pid' in  
Windows_Process_create. Are these of the same type? ie: Will the  
created process's getpid() return the same value as  
windows.c:Windows_Process_create?
>  - evol-poll-mingw.patch
>
>  This patch implements the poll() function on MinGW using the select 
> () call (based on code adapted from SML/NJ sources by Mathew  
> Fluet). Then, the SML functions poll() and select() will work at  
> least for sockets which is better than nothing. There is no  
> miracle, they will still fail when we use them on pipe handles.
I guess we need Matthew to relicence this. ;-)
>  - evol-stub-mingw-error-message.patch
>
>  This patch updates the error message we get when a non-implemented  
> function is executed in order to include the name of the function.  
> It makes it easier to port to MinGW.
Handy.
>  - evol-waitpid-mingw.patch
>
> This patch provides a partial implementation of the waitpid()  
> function. It makes it easier to port an existing application to  
> MinGW. I would say the implementation is not very clean. It uses a  
> linked list which is allocated using “malloc” and could be replaced  
> with something else. It also does not provide support for waiting  
> on any child which seems to be possible using the Windows function  
> WaitForMultipleObjects. Anyway, it remains useful.
Again I'm concerned about this mixing of pids. cwait works with the  
pid from Create_Process. Does waitpid? I suspect this will break on  
cygwin, at least. On that platform the Create_Process will return a  
real windows pid, but the getpid/waitpid/etc are wrappers that use  
some other numbering scheme AFAICR.

There's more PID madness happening in the spawne.c changes. Why are  
the Windows_Process_*register methods added to cygwin.h but used in  
an #ifdef __MINGW32__?

I don't have access to a windows computer for the next few days, so I  
can't test this patch on cygwin. However, we need to make sure all  
these different pid types play together well on both mingw and  
cygwin, I think. More documentation strategically placed comments  
would help.
> - evol-dash-in-pathes-cygwin.patch
>
> This patch is designed to use ‘/’ as the path separator when  
> building a path on Cygwin. The idea behind the patch is to use ‘/’  
> when the path is relative or uses the Unix format and to use ‘\\’  
> when the path is an absolute Windows like path.
I'm not sure I understand the rationale. Could you give some examples  
of the old (broken) behaviour and the new correct one? Does this  
still satisfy the basis specification?
> - evol-new-bitarray-functions.patch
>
> We have added a few extensions to the BitArray structure in smlnj- 
> lib. Implementing what we needed outside the compiler using the  
> existing public signature was not efficient enough. You may want to  
> consider this as a PolySpace specific need anyway.
No comment. I've never used SML/NJ once. :-)