GDI_TABLE_ENTRY structure

[Timo Kreuzer]

Our current gdi handle and handle table entry code uses a bunch of 
macros, masks, bitshifts and even hardcoded values, iirc.
This leads imo to errors, will make the code less portable and cannot 
make use of better compiler optimizations.
So I have created a new GDI_TABLE_ENTRY struct and a new type GDIHANDLE. 
Both use unions and packed structs to
directly access all fields. GDI_TABLE_ENTRY is completely compatible to 
our current one, so all the code would still be valid.

+ the compiler can optimize access of single elements better (movzx, 
byte ptr ...)
+ no more need for additional masks, shifts and macros
+ no more messed up code because of wrong hardcoded values
+ better readable code
+ probably easier porting to 64 bits

- must be redefined for big-endian systems
- direct access of stockbit is slightly slower, but could still be 
handled by a macro

#pragma pack(push,1)
typedef struct
{
    PVOID KernelData; /* Points to the kernel mode structure */
    union
    {
        DWORD ProcessId; /* process id that created the object, 0 for 
kernel objects */
        struct
        {
            unsigned ProcessId16: 16;
            unsigned Unused: 16;
        };
    };
    union
    {
        ULONG Type;
        struct
        {
            union
            {
                unsigned TypeLower: 16;
                struct
                {
                    union
                    {
                        unsigned HandleTypeStock: 8;
                        struct
                        {
                            unsigned HandleType: 7;
                            unsigned StockBit: 1;
                        };
                    };
                    unsigned ReuseCount: 8;
                };
            };
            union
            {
                unsigned TypeUpper: 16;
                struct
                {
                    unsigned StorageType: 8; /* PEN uses same value as 
BRUSH here, no stock bit */
                    unsigned Flags: 8;
                };
            };
        };
    };
    PVOID UserData;   /* Points to the user mode structure, usually NULL 
though */
} GDI_TABLE_ENTRY, *PGDI_TABLE_ENTRY;;

typedef union
{
    HGDIOBJ Handle;
    struct
    {
        unsigned Index: 16;
        union
        {
            unsigned Upper: 16;
            struct
            {
                union
                {
                    unsigned HandleTypeStock: 8;
                    struct
                    {
                        unsigned HandleType: 7;
                        unsigned StockBit: 1;
                    };
                };
                unsigned ReuseCount: 8;
            };
        };
    };
} GDIHANDLE, *PGDIHANDLE;
#pragma pack(pop)

[James Tabor]

Hi!
Timo Kreuzer wrote:
> #pragma pack(push,1)
> typedef struct
> {
>     PVOID KernelData; /* Points to the kernel mode structure */
>     union
>     {
>         DWORD ProcessId; /* process id that created the object, 0 for 
> kernel objects */
>         struct
>         {
>             unsigned ProcessId16: 16;
>             unsigned Unused: 16;
>         };
>     };
>     union
>     {
>         ULONG Type;
>         struct
>         {
>             union
>             {
>                 unsigned TypeLower: 16;
>                 struct
>                 {
>                     union
>                     {
>                         unsigned HandleTypeStock: 8;
>                         struct
>                         {
>                             unsigned HandleType: 7;
>                             unsigned StockBit: 1;
>                         };
>                     };
>                     unsigned ReuseCount: 8;
>                 };
>             };
>             union
>             {
>                 unsigned TypeUpper: 16;
>                 struct
>                 {
>                     unsigned StorageType: 8; /* PEN uses same value as 
> BRUSH here, no stock bit */
>                     unsigned Flags: 8;
>                 };
>             };
>         };
>     };
>     PVOID UserData;   /* Points to the user mode structure, usually NULL 
> though */
> } GDI_TABLE_ENTRY, *PGDI_TABLE_ENTRY;;
>
> typedef union
> {
>     HGDIOBJ Handle;
>     struct
>     {
>         unsigned Index: 16;
>         union
>         {
>             unsigned Upper: 16;
>             struct
>             {
>                 union
>                 {
>                     unsigned HandleTypeStock: 8;
>                     struct
>                     {
>                         unsigned HandleType: 7;
>                         unsigned StockBit: 1;
>                     };
>                 };
>                 unsigned ReuseCount: 8;
>             };
>         };
>     };
> } GDIHANDLE, *PGDIHANDLE;
> #pragma pack(pop)

Okay! 8^D
James

[Ged]

Timo Kreuzer wrote:e
> So I have created a new GDI_TABLE_ENTRY struct and a new type GDIHANDLE.

Looks good, a thumbs up from me  :)

1 comment:
> #pragma pack(push,1)

#include <pshpack1.h>
> #pragma pack(pop)

#include <poppack.h>

Ged.

This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message, which arise as a result of e-mail transmission. If verification is required please request a hard-copy version.

Amteus Secure Communications Ltd
57 Cardigan Lane,
Leeds, 
LS4 2LE
t: [phone]
f: [phone]

Registered in England No 4760795

http://www.amteus.com

[Alex Ionescu]

I disagree.

On 8/30/07, Ged [e-mail] wrote:
> Timo Kreuzer wrote:e
> > So I have created a new GDI_TABLE_ENTRY struct and a new type GDIHANDLE.
>
> Looks good, a thumbs up from me  :)
>
> 1 comment:
> > #pragma pack(push,1)
>
> #include <pshpack1.h>
> > #pragma pack(pop)
>
> #include <poppack.h>
>
> Ged.
>
> This message contains confidential information and is intended only for
> the individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and delete
> this e-mail from your system. E-mail transmission cannot be guaranteed to be
> secure or error-free as information could be intercepted, corrupted, lost,
> destroyed, arrive late or incomplete, or contain viruses. The sender
> therefore does not accept liability for any errors or omissions in the
> contents of this message, which arise as a result of e-mail transmission. If
> verification is required please request a hard-copy version.
>
> Amteus Secure Communications Ltd
> 57 Cardigan Lane,
> Leeds,
> LS4 2LE
> t: [phone]
> f: [phone]
>
> Registered in England No 4760795
>
> http://www.amteus.com
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Best regards,
Alex Ionescu

[Timo Kreuzer]

Surely we all trust you and you are probably right, but if you want me 
to be *convinced*, you will hopefully answer my 3 questions:
1.) With what exactly do you disagree?
2.) Why do you disagree?
3.) Do you have a better suggestion?

ps: I assume that you have been in a hurry, that didn't allow you to be 
more specific in the first instance.

Regards,
Timo

Alex Ionescu schrieb:
> I disagree.
>
> On 8/30/07, *Ged* <[e-mail] 
> mailto:[e-mail]> wrote:
> Timo Kreuzer wrote:e
>
> > So I have created a new GDI_TABLE_ENTRY struct and a new type
> GDIHANDLE.
>
> Looks good, a thumbs up from me  :)
>
> 1 comment:
>
> > #pragma pack(push,1)
> #include <pshpack1.h>
>
> > #pragma pack(pop)
> #include <poppack.h>
>
> Ged.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> This message contains confidential information and is intended
> only for the individual named. If you are not the named addressee
> you should not disseminate, distribute or copy this e-mail. Please
> notify the sender immediately by e-mail if you have received this
> e-mail by mistake and delete this e-mail from your system. E-mail
> transmission cannot be guaranteed to be secure or error-free as
> information could be intercepted, corrupted, lost, destroyed,
> arrive late or incomplete, or contain viruses. The sender
> therefore does not accept liability for any errors or omissions in
> the contents of this message, which arise as a result of e-mail
> transmission. If verification is required please request a
> hard-copy version.
>
> Amteus Secure Communications Ltd
> 57 Cardigan Lane,
> Leeds,
> LS4 2LE
> t: [phone]
> f: [phone]
>
> Registered in England No 4760795
>
> http://www.amteus.com <http://www.amteus.com>
>
>
> _______________________________________________
> Ros-dev mailing list
> [e-mail] <mailto:[e-mail]>
> http://www.reactos.org/mailman/listinfo/ros-dev
> <http://www.reactos.org/mailman/listinfo/ros-dev>
>
>
>
>
> -- 
> Best regards,
> Alex Ionescu
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[James Tabor]

Alex Ionescu wrote:
> I disagree.
>
> On 8/30/07, *Ged* <[e-mail] 
> mailto:[e-mail]> wrote:
> Timo Kreuzer wrote:e
>
>  > So I have created a new GDI_TABLE_ENTRY struct and a new type
> GDIHANDLE.
>
> Looks good, a thumbs up from me  :)
>
> 1 comment:
>
>  > #pragma pack(push,1)
> #include <pshpack1.h>
>
>  > #pragma pack(pop)
> #include <poppack.h>
>
> Ged.

Oh I see,,, The structures are okay but using them like a macro, no.
James

[Alex Ionescu]

Hey,

Yes I was in a hurry, sorry.

1) a. Modifying a well known, Microsoft-documented structure.
     b. Using structure-based bit logic instead of shifts and masks.
2) a. I think this is clear.
     b. I think it makes the code less maintainable, harder to
understand, less clean, and potentially hurts performance. This
becomes a significantly bigger problem when thinking about other
architectures with alignment requirements, or different endianness.
3) Use macros to hide away the mask/offsets, or better yet, inlined functions.

On 8/30/07, James Tabor
[e-mail] wrote:
> Alex Ionescu wrote:
> > I disagree.
> >
> > On 8/30/07, *Ged* <[e-mail]
> > mailto:[e-mail]> wrote:
> > Timo Kreuzer wrote:e
> >
> >  > So I have created a new GDI_TABLE_ENTRY struct and a new type
> > GDIHANDLE.
> >
> > Looks good, a thumbs up from me  :)
> >
> > 1 comment:
> >
> >  > #pragma pack(push,1)
> > #include <pshpack1.h>
> >
> >  > #pragma pack(pop)
> > #include <poppack.h>
> >
> > Ged.
>
> Oh I see,,, The structures are okay but using them like a macro, no.
> James
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Best regards,
Alex Ionescu

[James Tabor]

Alex Ionescu wrote:
> Hey,
>
> Yes I was in a hurry, sorry.
> a. Modifying a well known, Microsoft-documented structure.
>   b. Using structure-based bit logic instead of shifts and masks.
> a. I think this is clear.
>   b. I think it makes the code less maintainable, harder to
>
> understand, less clean, and potentially hurts performance. This
> becomes a significantly bigger problem when thinking about other
> architectures with alignment requirements, or different endianness.
> 3) Use macros to hide away the mask/offsets, or better yet, inlined functions.

Could we use _X86_ for one structure type and have the other too?
Thanks,
James

[Alex Ionescu]

I don't know --maybe--, but then you end up with dozens of structure
definitions.

Bitfields are simply a bad idea in this scenario because some of the
code needs to be performant, and you can't trust the compiler to
generate the best possible code to access a member. With a mask/shift,
you can. There's been lots of codegen errors by GCC in the past due to
bitfields. I just talked with someone in my office who agrees on this
issue, having worked with GCC pretty much since day one :)

Also, treating the various GDI objects as unions is also pretty wrong
-- they're not unions anymore, as the structure *changes* depending on
the object. This means you'll be passing around a union, and all the
code will have to check its type. By using separate containers for
each object, you have type-safe code and more efficient design.

On 8/30/07, James Tabor
[e-mail] wrote:
> Alex Ionescu wrote:
> > Hey,
> >
> > Yes I was in a hurry, sorry.
> > a. Modifying a well known, Microsoft-documented structure.
> >   b. Using structure-based bit logic instead of shifts and masks.
> > a. I think this is clear.
> >   b. I think it makes the code less maintainable, harder to
> >
> > understand, less clean, and potentially hurts performance. This
> > becomes a significantly bigger problem when thinking about other
> > architectures with alignment requirements, or different endianness.
> > 3) Use macros to hide away the mask/offsets, or better yet, inlined functions.
>
> Could we use _X86_ for one structure type and have the other too?
> Thanks,
> James
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Best regards,
Alex Ionescu

[Timo Kreuzer]

Alex Ionescu schrieb:
> I don't know --maybe--, but then you end up with dozens of structure
> definitions.
>
> Bitfields are simply a bad idea in this scenario because some of the
> code needs to be performant, and you can't trust the compiler to
> generate the best possible code to access a member. With a mask/shift,
> you can. There's been lots of codegen errors by GCC in the past due to
> bitfields. I just talked with someone in my office who agrees on this
> issue, having worked with GCC pretty much since day one :)

Ok, bitfields and packing is probably not a good idea. When I remove the 
stockbit and  replace the bitfields with BYTE and WORD then we would 
need no pack, as they are aligned according to their size. (Or are there 
archs that require higher alignment (like 4 byte alignment for a BYTE)?)
And I thought that the compiler should be better at optimizing access to 
individual fields by using movzx and byte ptr...
> Also, treating the various GDI objects as unions is also pretty wrong
> -- they're not unions anymore, as the structure *changes* depending on
> the object. This means you'll be passing around a union, and all the
> code will have to check its type. By using separate containers for
> each object, you have type-safe code and more efficient design.

You know we are talking about the handle table entries, ya? I don't see 
where the structure should change. TypeInfo is TypeInfo and ReuseCounter 
is ReuseCounter if we have a HBITMAP or a HPEN or whatever.
> On 8/30/07, James Tabor
> [e-mail] wrote:
> > Alex Ionescu wrote:
> > > Hey,
> > >
> > > Yes I was in a hurry, sorry.
> > > a. Modifying a well known, Microsoft-documented structure.

Well known, yes, but all structures I have seen so far are more or less 
like in Yuan's book and use WORDS and BYTEs.
Example: 
http://www.sstorm.cn/html/Exploits/20070421/MS_Windows_GDI_Local_Privilege_E...
Or do you have access to ms "documentation" that other people don't have?
And I modified it only to be more like the struct from Yuan's book.
> > >  b. Using structure-based bit logic instead of shifts and masks.
> > > a. I think this is clear.
> > >   b. I think it makes the code less maintainable, harder to
> > >
> > > understand, less clean, and potentially hurts performance. This
> > > becomes a significantly bigger problem when thinking about other
> > > architectures with alignment requirements, or different endianness.
> > > 3) Use macros to hide away the mask/offsets, or better yet, inlined functions.

I think that the resulting code would be better readable. Accessing a 
member of a struct seems to be more clear than using some macros, but 
that might be a matter of taste. We already have a bunch of macros and 
would need a lot more to handle all operations.

One idea was that it should be faster (at least on x86), so I put the 
different implementations in a big loop and measured time
This is the result:

Create a handle from index and type:
- hobj[i&0xf] = (HGDIOBJ)((i & 0xFFFF) | (TypeInfo << 16)); // 2032 ticks
- h[i&0xf].Index = i; h[i&0xf].Upper = TypeInfo; // 2032 ticks

Modify the reuse counter:
- buf[i&0xf].ReuseCount = i; // 2031 ticks
- buf[i&0xF].Type = ((i & 0x0000ff00) | (buf[i&0xf].Type & 0xffff00ff)); 
//2031 ticks

looks like gcc can optimize bitmasks and shifts pretty good.

I don't know how it would behave on any other arch, but the optimization 
on x86 was what I had hoped to achieve. This idea just died ;-)

So I'd be fine with keeping it as it is

Regards,
Timo

ps: I have another idea that should boost performance of gdi/user a lot 
more...

[Timo Kreuzer]

What do you mean with "using them lika a macro"?

James Tabor schrieb:
> Oh I see,,, The structures are okay but using them like a macro, no.
> James
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[James Tabor]

Hi!
Timo Kreuzer wrote:
> What do you mean with "using them lika a macro"?

Push and pop,,, was that a macro?
Sorry,
James

[Timo Kreuzer]

no that replaces only the #pragma pack(1)

James Tabor schrieb:
> Hi!
> Timo Kreuzer wrote:
> > What do you mean with "using them lika a macro"?
>
> Push and pop,,, was that a macro?
> Sorry,
> James
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Magnus Olsen]

Okay for me to, we need it :)

----- Original Message ----- 
From: "Ged" [e-mail]
To: "'ReactOS Development List'" [e-mail]
Sent: Thursday, August 30, 2007 2:46 PM
Subject: Re: [ros-dev] GDI_TABLE_ENTRY structure
> Timo Kreuzer wrote:e
> > So I have created a new GDI_TABLE_ENTRY struct and a new type GDIHANDLE.
>
> Looks good, a thumbs up from me  :)
>
> 1 comment:
> > #pragma pack(push,1)
>
> #include <pshpack1.h>
> > #pragma pack(pop)
>
> #include <poppack.h>
>
> Ged.
>
> This message contains confidential information and is intended only for

the individual named. If you are not the named addressee you should not
disseminate, distribute or copy this e-mail. Please notify the sender
immediately by e-mail if you have received this e-mail by mistake and delete
this e-mail from your system. E-mail transmission cannot be guaranteed to be
secure or error-free as information could be intercepted, corrupted, lost,
destroyed, arrive late or incomplete, or contain viruses. The sender
therefore does not accept liability for any errors or omissions in the
contents of this message, which arise as a result of e-mail transmission. If
verification is required please request a hard-copy version.
> Amteus Secure Communications Ltd
> 57 Cardigan Lane,
> Leeds,
> LS4 2LE
> t: [phone]
> f: [phone]
>
> Registered in England No 4760795
>
> http://www.amteus.com
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Magnus Olsen]

I forget the GDI table are share and can be extract from  GdiQueryTable,
then use the decoding metod that being desc in yuan book.   GdiQueryTable
return the memory pointer of  gdi_table.
so it is bit bad idea. after rethink.

----- Original Message ----- 
From: "Magnus Olsen" [e-mail]
To: "ReactOS Development List" [e-mail]
Sent: Thursday, August 30, 2007 7:28 PM
Subject: Re: [ros-dev] GDI_TABLE_ENTRY structure
> Okay for me to, we need it :)
>
> ----- Original Message ----- 
> From: "Ged" [e-mail]
> To: "'ReactOS Development List'" [e-mail]
> Sent: Thursday, August 30, 2007 2:46 PM
> Subject: Re: [ros-dev] GDI_TABLE_ENTRY structure
> > Timo Kreuzer wrote:e
> > > So I have created a new GDI_TABLE_ENTRY struct and a new type

GDIHANDLE.
> > Looks good, a thumbs up from me  :)
> >
> > 1 comment:
> > > #pragma pack(push,1)
> >
> > #include <pshpack1.h>
> > > #pragma pack(pop)
> >
> > #include <poppack.h>
> >
> > Ged.
> >
> > This message contains confidential information and is intended only for
>
> the individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and

delete
> this e-mail from your system. E-mail transmission cannot be guaranteed to

be
> secure or error-free as information could be intercepted, corrupted, lost,
> destroyed, arrive late or incomplete, or contain viruses. The sender
> therefore does not accept liability for any errors or omissions in the
> contents of this message, which arise as a result of e-mail transmission.

If
> verification is required please request a hard-copy version.
> > Amteus Secure Communications Ltd
> > 57 Cardigan Lane,
> > Leeds,
> > LS4 2LE
> > t: [phone]
> > f: [phone]
> >
> > Registered in England No 4760795
> >
> > http://www.amteus.com
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Timo Kreuzer]

The structure is completely binary and source compatible to what we have 
so far, wich is (now, after my fix to gdiobj code) compatible to windows.
We can simply replace the struct and everything else could stay the same.
It just gives us the possibility to access single fields without using 
masks and bitshifts.
And it has the Fields TypeLower in the table entry type and Upper in the 
handle type. This allows quick comparison.
    if (Entry->TypeLower == Handle.Upper)
instead of
    if ((Entry->Type << GDI_ENTRY_UPPER_SHIFT) == GDI_HANDLE_GET_TYPE(hObj))

Magnus Olsen schrieb:
> I forget the GDI table are share and can be extract from  GdiQueryTable,
> then use the decoding metod that being desc in yuan book.   GdiQueryTable
> return the memory pointer of  gdi_table.
> so it is bit bad idea. after rethink.

[David Johnson]

Any word on ReactOS PPC?
On Aug 30, 2007, at 1:18 PM, Timo Kreuzer wrote:
> The structure is completely binary and source compatible to what we  
> have
> so far, wich is (now, after my fix to gdiobj code) compatible to  
> windows.
> We can simply replace the struct and everything else could stay the  
> same.
> It just gives us the possibility to access single fields without using
> masks and bitshifts.
> And it has the Fields TypeLower in the table entry type and Upper  
> in the
> handle type. This allows quick comparison.
>     if (Entry->TypeLower == Handle.Upper)
> instead of
>     if ((Entry->Type << GDI_ENTRY_UPPER_SHIFT) ==  
> GDI_HANDLE_GET_TYPE(hObj))
>
> Magnus Olsen schrieb:
> > I forget the GDI table are share and can be extract from   
> > GdiQueryTable,
> > then use the decoding metod that being desc in yuan book.    
> > GdiQueryTable
> > return the memory pointer of  gdi_table.
> > so it is bit bad idea. after rethink.
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

_________________________________
Dave Johnson
http://www.davefilms.us DaveFILMS®
San Diego, CA & Nashville, TN
http://perditioncity.us/
Voice Talent
Writer, Producer, Director
Independent Audio Theater Producer
Voice mail and Fax#:[phone]
Voice [phone]
Skype & Msn Live messanger "DaveFilms"
AIM / iChat AV "DaveFilms2007"