fix 64bit two-stage allocator on Solaris/AMD64 platform (#10790)
ClosedPublic

Authored by kgardas on Aug 24 2015, 3:46 PM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kgardas updated this revision to Diff 3913.Aug 24 2015, 3:46 PM
kgardas retitled this revision from to fix 64bit two-stage allocator on Solaris/AMD64 platform (#10790).
kgardas updated this object.
kgardas edited the test plan for this revision. (Show Details)
ezyang accepted this revision.Aug 24 2015, 3:53 PM
ezyang edited edge metadata.

I don't know enough Solaris to evaluate the correctness of this patch, but if it works, ship it!

But a comment would be nice...

austin added inline comments.Aug 24 2015, 5:56 PM
rts/posix/OSMem.c
165

irix support?

suchwow

In D1169#32399, @ezyang wrote:

But a comment would be nice...

Yeah, I can't really understand or quickly find any references to this here, although it seems simply mapping /dev/zero is the proper way to get anonymous/private memory (sometimes?). It would be nice to back it up with a link to man mmap and man zero maybe?

Indeed, I

In D1169#32401, @austin wrote:
In D1169#32399, @ezyang wrote:

But a comment would be nice...

Yeah, I can't really understand or quickly find any references to this here, although it seems simply mapping /dev/zero is the proper way to get anonymous/private memory (sometimes?). It would be nice to back it up with a link to man mmap and man zero maybe?

Indeed, I seem to recall that some platforms used /dev/zero mappings in this way. I too am failing to find any references for this, however.

bgamari accepted this revision.Aug 25 2015, 2:15 AM
bgamari edited edge metadata.

Guys, thanks for your comments. I'd like to address them here:

  • Irix support: the thing also surprised me in the original code and was the reason why I forked Solaris from solaris||irix ifdef block to make sure I do not break irix thing.
  • the idea behind whole this fix is that Solaris requires for whatever reason to really allocate anon mem for us. In the second case of the original code this was not true as MAP_ANON was missing. Please note that I'm not able to quickly find why exactly we need to have anon mem here, but well, it's working this way
  • there are exactly two ways how to get anon mem on Solaris, either use /dev/zero as in the code above or simply use MAP_ANON and -1 as file descriptor:
ret = mmap(addr, size, prot, flags | MAP_ANON | MAP_PRIVATE, -1, 0);

is working well too. If this is more close to common mmap understanding than I can update the patch to use this second way instead of /dev/zero above and then perhaps avoid a need for the comment?
Solaris `man mmap' claims this clearly:

When MAP_ANON is set in flags, and  fildes  is  set  to  -1,
mmap()  provides  a direct path to return anonymous pages to
the caller.  This operation is equivalent to passing  mmap()
an  open  file  descriptor on /dev/zero with MAP_ANON elided
from the flags argument.
kgardas updated this revision to Diff 3923.Aug 25 2015, 7:16 AM
kgardas edited edge metadata.

Solaris does not need any special case for mmap handling since it supports well the common case already presented in the code.

austin accepted this revision.Aug 25 2015, 2:14 PM
austin edited edge metadata.

Nice! Sounds legit to me. Even better than adding a block of code is just enabling one that already worked. :)

This revision is now accepted and ready to land.Aug 25 2015, 2:14 PM
This revision was automatically updated to reflect the committed changes.