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

[Matthew Fluet]

>>> - bug-fix-cygwin-1_3_17.patch :
>> I question whether these should be applied. 
>>
>> Perhaps we could instead offer this patch as a separate download?
> 
> The problem with Cygwin is that it is not possible to run concurrently
> multiple versions of the DLL. As a consequence, if you have a product which
> requires a specific version and an other product which requires a more
> recent one, you get into troubles.
> 
> Another reason why I like this particularly old version of Cygwin is that
> we've noticed that the newer versions come with some performance
> regressions.
> 
> Anyway, as we plan to stop using Cygwin and run through MinGW in the future,
> I'm not so interested in having this patch included in the official release
> of the MLton compiler.

I think it would be fine to include portion of the patch that includes 
work-arounds for fpclassify and nanosleep.  This would allow a program 
compiled by MLton to be run on older Cygwin releases, without 
necessarily condoning development on that older Cygwin release.

>>> - bug-fix-solaris7.patch
>>>
>> Does this MAP_ANON value actually work? If so, is it declared in
>> another header? If not, why?
> 
> As a consequence, the value we set for MAP_ANON (I used the value the
> Solaris 8 headers set) is simply ignored when the function is executed on
> Solaris 7 and will be taken into account on more recent versions. We have
> tested our binaries on both Solaris 7 and 8 and they work fine.

I figured you had tested it.  It just seems that 0 is a safer value for 
a flag, as OR-ing it in obviously has no effect.  (I could imagine that 
some system calls raise an error if they are passed an unknown flag, but 
obviously mmap on Solaris 7 is not such a function.)

>>>  - bug-fix-fullpath-windows.patch
>>
>> 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?
> 
> Sorry, I based the implementation on a piece of code we used when we built
> our product using the 20030716 version of MLton (the fullPath implementation
> was broken on this version and we had included and modified the one from a
> newer release directly in our code). I could not manage to find the reason
> why we had done this.
> 
> Maybe the exception which is raised when we run isLink on a stale NFS file
> system is not a SysErr exception and we wanted to change that but I'm afraid
> it's going to be hard to reproduce.

As I said in my other message, I didn't apply this portion of the patch. 
      It seems that it is orthogonal to the MinGW port, and nobody 
really knows what it is for anymore.

>>>  - bug-fix-nethostdb-mingw.patch
>> 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?
> 
> I guess the idea is to avoid loading the WinSock2 DLL when the binary does
> not need it. Loading a DLL is not "free". Do you think it could be possible
> to call the function from SML code and rely on the dead code elimination in
> order to achieve the same result ?

There are different trade-offs.

In basis-library/net/net-host-db.sml, you could include code like the 
following:

structure Prim =
struct
   open Prim
   val (getByAddress, getByName, getHostName) =
     (if let open Primitive.MLton.OS in host = MinGW end
         then Net.init ()
         else ()
      ; (getByAddress, getByName, getHostName))
end

This would have the effect that if any of 'getByAddress', 'getByName', 
or 'getHostName' were used in the user program, then, at program 
startup, Net.init would be called just once.  (One could pull the same 
trick with the Socket.* functions.)  And you don't pay the cost of 
checking whether networking has been initialized at each use of a 
networking function.

On the other hand, this means that one will load WinSock2 DLL regardless 
of whether or not the program dynamically uses networking functions.

>>>  - evol-poll-mingw.patch
>> I guess we need Matthew to relicence this. ;-)
> 
> While surfing on the internet, I've seen a number of articles which mention
> that on some systems, the poll() system function may use more resources than
> the select() call. I'm not sure anyway but maybe it would be a good idea to
> update the compiler in order to really implement the SML select function
> using the select() call.  

I think this is probably the right way to go.  I'm certain that it 
wouldn't be too hard, and it is probably better than faking 'select' 
with 'poll'.

>>> - evol-getpid-mingw.patch
> 
>>>  - evol-waitpid-mingw.patch

I think these are useful evolutions, but I don't currently have MinGW 
set up to test them.  It would also be useful to hear from others who 
have more MinGW/Windows experience.

>>> - 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?
> 
> As far as I remember, if we run a function from the Path structure on a
> Cygwin path, the '/' in the source path used to be converted to '\\' even
> when the source path was given in Posix format except the first dash in case
> of an absolute path (/usr/local -> /usr\local). 

It is certainly the case that the semantics of OS.Path.* functions (on 
Cygwin and MinGW) have changed since 20030716, but they have not changed 
20051202.  Looking at the SVN log, I see that a few of those changes 
were committed on behalf of Wesley.

> Then, if we use the resulting path in a shell command, the command may fail
> because the backslash will be interpreted as an escape sequence if we don't
> pay attention to escape the string before. Usually, the ones who write code
> or scripts for Cygwin are more familiar with Unix than Windows and do not
> think they have to pay attention to such things and may be disappointed.

I appreciate the difference between Windows and Unix paths.  I'm just 
not sure whether your example above (/usr/local -> /usr\local) is an 
example of a desired conversion or is an example of a conversion you 
believe is wrong.