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

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> 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...
>

All right. ^^

>> +#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.
>

Ok.

>> +#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.
>

Absolutely.

>> +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?
>

Because cb is used as in/out, but fl as out only.

>> +#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?
>

Of course for debugging.

>> +    // 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.
>

However the output is 'WARNING! Invalid Ptr handle!'.

>> +#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.
>

No problem. but I should split #ifdef __OS2__ into two parts. hmm...

>> @@ -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.
>

Unforunately it does not work on OS/2. it causes MPlayer crashed. And I 
don't understand how hard-coded address value, 0x62b67ae0, can be used.

>> @@ -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.

Fixed.

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 2.0
Under OS/2 Warp 4 for Korean with FixPak #15
On Intel Core2Duo T5500 1.66 GHz with 1 GB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: loader.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100209/9ab91cb2/attachment.asc>