[MPlayer-dev-eng] [PATCH] Win32 loader support for OS/2

[Reimar Döffinger]

On Fri, Feb 05, 2010 at 08:53:25PM +0900, KO Myung-Hun wrote:
> This patch adds Win32 loader support for OS/2.

Sorry, I'm a bit busy...

> +#ifdef __OS2__
> +    /* convert flat addr to sel idx for LDT_SEL() */
> +    fs_ldt = ( unsigned int )(( unsigned long )fs_seg  >> 16 );

I fail to see the purpose of so many casts.
fd_ldt = (uintptr_t)fs_seg >> 16;
seems more like it to me.

> +#ifdef __OS2__
> +uint32_t _System DosQueryMem(void *, uint32_t *, uint32_t *);

While I agree with Diego that it is ugly, I think getting loader and
OS/2 not to conflict so you can include OS/2 headers is probably
too much work to be worth it.

> +static int is_invalid_ptr(void *p)
> +{
> +    uint32_t cb = 1;
> +    uint32_t fl;
> +
> +    if( DosQueryMem(p, &cb, &fl))

Why intializing cb and not fl? Is the DosQueryMem behaving that
strange or is that unintentional?

> +#ifdef DEBUG_QTX_API
> +    printf("addr = %p, cb = %d, fl = %08X : ", p, cb, fl);
> +    if(fl & 0x10)
> +        printf("COMMIT ");
> +    if(fl & 0x2000 )
> +        printf("SHARED ");
> +    if(fl & 0x4000)
> +        printf("FREE ");
> +    if(fl & 0x10000)
> +        printf("BASE ");
> +    if(fl & 0x01)
> +        printf("READ ");
> +    if(fl & 0x02)
> +        printf("WRITE ");
> +    if(fl & 0x04)
> +        printf("EXEC ");
> +    if(fl & 0x08)
> +        printf("GUARD ");
> +    printf("\n");
> +#endif

is that necessary?

> +    // Occasionally, ptr with 'EXEC' attr is passed.
> +    // On OS/2, however, malloc() never set 'EXEC' attr.
> +    // So ptr with 'EXEC' attr is invalid.
> +    if(fl & 0x04)
> +        return 1;

If it's "EXEC" the pointer is not invalid, it does not point
to heap memory though.
So is_allocated_ptr or is_head_ptr or something like that would
be a better name for the function I think.

> +#else
> +#define is_invalid_ptr(p) ((uint32_t)(p) >= 0x60000000)

Use a function for both cases, only replace the function body,
that's IMO nicer than a lowercase macro.

> @@ -758,6 +802,7 @@
>    int plen=-1;
>    // find the code:
> 
> +#ifndef __OS2__
>    dptr=0x62b67ae0;dptr+=2*((reg->eax>>16)&255);
>  //  printf("FUNC: flag=%d ptr=%p\n",dptr[0],dptr[1]);
>    if(dptr[0]&255){
> @@ -792,6 +837,7 @@
>  	  pwrapper=dptr[1];
>        }
>    }
> +#endif

That whole point is under
DEBUG_QTX_API, which is disabled by default, so IMO don't disable parts
of it doubly - someone who actually needs the debug code can get it
to compile at least as fast as figure out why half of the output is
missing on OS/2 only.

> @@ -844,7 +890,7 @@
>  #endif
>        return 1;
>    case 0x15002f: //DisposePtr
> -      if(((uint32_t *)stack_base)[1]>=0x60000000)
> +      if(is_invalid_ptr((void *)((uint32_t *)stack_base)[1]))

I know the code below does the same thing, but shouldn't this be
> if(is_invalid_ptr(((void **)stack_base)[1]))
If that works, feel free to change the free below to use that style
right away.