[MPlayer-dev-eng] [PATCH] (loader/ext.c) VirtualAlloc() SIGSEGV on Linux + other fixes

[A. Guru]

* On Tuesday 2004-08-24 at 21:59:49 +1000, Robert Lunnon wrote:
> 
> The fact of the matter is that this hint IS always ignored under Solaris x86 
> (well < 10 anyway). Not only that the memory will ordinarily be allocated 
> above d0000000 whereas windows expects everything to be below the 3GB mark, 
> normally this doesn't hurt but Alexandre does tell me some windows processes 
> are sensitive to this behaviour. He was however not specific about which 
> ones.

I see.  That means the following situation is possible:  the pages a DLL
would like are already taken (first call to VirtualAlloc will return
NULL), the pages Solaris would give it (second call to VirtualAlloc
with NULL addr) are not acceptable, but there are other pages that are
available and acceptable, only we have to find them.

> > The other pertinent question is:  (2) is there any MS Windows DLL used
> > by mplayer that can't be relocated?
> 
> Dunno about that, can't say I know a lot about Windows, the caveats are the 
> same Solaris kernel/user split is different to Linux and Windows so it will 
> locate maps above where the dlls will expect to be located unless you call 
> MMAP_FIXED

Nevertheless, I have now checked all the files in

  <http://www.mplayerhq.hu/MPlayer/releases/codecs/all-20040809.tar.bz2>

with "objdump -x".

The *.so* and *.xa files are native Linux ELF and so are not relevant
here.  All the other files (*.acm, *.ax, *.dll, *.DLL, *.drv, *.qts,
*.qtx, and *.vwp) are MS Windows files and they all have relocation
information.  This means Solaris, or any POSIX operating system for that
matter, would not have a functionality problem with the patch that I am
proposing, if only the MS Windows DLLs were not sensitive about this 3 GB
limit you mention above.  One problem it will have is a performance penalty
one if an unnecessary mmap/munmap pair and relocation are performed (but
no worse than anything else like the multiple mincore syscalls done by
Wine for Solaris/NetBSD).

I don't know how to check all those MS Windows file for a 3 GB problem,
especially since I don't have multimedia files of all these different
formats at hand.

> > If yes to question 2 and no to question 1, then someone will have to write
> > an initVirtualAlloc()-type solution that understands Solaris specific
> > /proc filesystem binary files /proc/self/map and /proc/self/rmap (or just
> > those DLLs won't work).  Is this the case?  If it is, then by all means
> > please write such a solution for Solaris with the proper #ifdefs.  Such a
> > solution does have a performance penalty and problems with long-running
> > gmplayer processes.  A solution that checks /proc every single time will
> > have an even bigger performance penalty, but no problem with long-running
> > gmplayer processes.
> >
> > If yes to both questions, then there is no obvious solution anyway.
> > That's true for all OSes.  There are _very_ non-obvious solutions for
> > several OSes (want to play tricks with the dynamic loader?).
> 
> There is a solution for Solaris already implemented in Wine which involves 
> checking the use of the memory space before mapping to emulate the linux 
> behaviour. I assume this is what you are referring to ?

I was refering to the initVirtualAlloc() solution (mainly for Linux using
/proc/$$/maps) which was proposed in a previous mailing list thread
which is referenced elsewhere in this thread.  It's not aware of the 3 GB
problem either.

I just checked the code in Wine.  It doesn't rely on /proc at all
(for mmap stuff).  The part to get the address on Solaris/NetBSD if it
is available and under limit (just like on Linux) is fairly small and
relies on mincore, vfork, and ignoring SIGCHLD.  The part where addr==0
(usually on a second VirtualAlloc call when the first had to fail) and
we want to return something under limit is a lot more complicated and
spread in several files.

I now wonder why mplayer doesn't just use all the mmap, VirtualAlloc, and
NtAllocateVirtualMemory code from Wine.  The MEM_RESERVE and MEM_COMMIT
semantics of VirtualAlloc in mplayer are not fully implemented anyway.
And Wine knows about the MAP_TRYFIXED flag (that I didn't know about until
now) when available.  Given the complexity involved in the non-Linux case,
I now don't see the point of reinventing Wine's wheel.

> > If no to question 2 and yes to question 1, then Solaris will incur nothing
> > else than the same performance penalty that any OS that doesn't implement
> > something like MAP_NOUNMAP.
> >
> > If no to both questions, then Solaris will just incur a performance
> > penalty a little more often.  Whether this is better or worse than an
> > initVirtualAlloc()-type solution, which can still be written for this
> > case, remains to be benchmarked once this is done.
> 
> I can't say, I only commented to say that such a change might not be as 
> innocuous as you think, because mmap semantics do differ between OSes. I have 
> looked at this aspect of wine in the past and tested the original patch.

Indeed.  But the current mplayer situation where MAP_FIXED is used
blindly and may result in an implicit munmap is not acceptable on any OS.

So both the Linux and Solaris/NetBSD situations have to be handled.

> Assuming the semantics of the dlls are understood (IE what memory they will 
> want to map) isn't it then possible to preallocate the space

If the space they prefer is taken early on by ld.so (or equivalent)
for a native dynamic library when mplayer is started, then it's going
to be really hard to do something about it (i.e., preallocation).
We'll probably just have to settle for relocation.

> so that 
> performance hits occur when we want them to in problematic cases - those that 
> fail case 2 and need to call VirtualAlloc frequently .

I think most of my concerns about performance penalty don't matter much
now considering the necessary complexity of what has to be done in some
cases anyway.  Things can only be optimized for Linux is a very specific
case and even then, the gains pale in comparison.