[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ros-dev
Subject:    Re: [ros-dev] GDI_TABLE_ENTRY structure
From:       Timo Kreuzer <timo.kreuzer () web ! de>
Date:       2007-08-31 3:12:04
Message-ID: 46D78704.4000001 () web ! de
[Download RAW message or body]


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
> <jimtabor@adsl-64-217-116-74.dsl.hstntx.swbell.net> wrote:
> 
> > Alex Ionescu wrote:
> > 
> > > Hey,
> > > 
> > > Yes I was in a hurry, sorry.
> > > 
> > > 1) 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_Escalation_Exploit__MS07_017__2_537_2.html
 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.
> > > 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.
> > > 
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...

_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev


[prev in list] [next in list] [prev in thread] [next in thread]