Trap handler rewrite

[Timo Kreuzer]

No doubt the new code is a huge advantage over the old code. Still there
are some issues in my opinion.

Currently the trap handlers are generated inside C code by a macro
called KiTrap(), located in trap_x.h.
This macro creates a C function, that uses KiTrapStub inline function to
create the actual entry stub. KiTrapStub is 90% pure gcc inline assembly.
At the end it does a jmp to the real C handler.

Most C handlers will start processing with an inline function
KiEnterTrap, which is C code with inline assembly functions like
KiSetSaneSegments, Ke386GetFs. KiTrap06Handler/Trap0DHandler are
exceptions, they use KiIsV8086TrapSafe to determine in inline assembly
if we have a v86 trap.
In this case KiEnterV86Trap is used instead of KiEnterTrap. KiTrap02 is
also an exception, it's a pure C handler


Now what's wrong?

1.) The gcc inline assembly is very compiler specific and MSVC
unfriendly. The old method using GAS specific asm files was at least
linkable with MSVC code. The new code needs porting.

2.) The current C code makes assumptions about what the compiler will
do, ignoring the ABI, which requires that you do not change ds/es in C
code, not even temporarily and that the direction flag is clear on
function entry. The compiler might at any place use ds/es for whatever
it likes. It must be a valid flat segment. Only using stack variables
might work on the current gcc, but might fail on other compilers. A good
example would be MSVC's use of __security_cookie, which is referenced on
function entry through ds. I remember that not having cleared the
direction flag in a trap handler caused strange random bugs in the past,
we should avoid making this mistake again. KiTrapStub even changes esp
inside C code (in cases where the function is entered without valid esp)


Suggested modifications:

Implement the trap entry functions in a pure assembly file, using
assembly macros which would combine KiTrapStub and the segment handling
part of KiEnterTrap.
Trap exit could possibly be done this way as well. On the amd64 branch
the C handlers return a status value back to the assembly stub, but a
jump or call could be done as well.
It would be possible to use ML for these assembly files as well. This
works fine in the amd64 branch.
Also strictly stick to C ABI instead of using compiler extensions and
making assumptions about how the compiler works.
It would improve portability, make the code safe and also more readable.
And we would not really add more assembly, just move the current inline
assembly to a pure assembly file.
Possible performance disadvantages: I don't see any. We might end up
with a jmp or call instruction more, but we can possibly safe this
somewhere else.

And maybe finally someone deletes the disgusting Ke386SetDs()... ;-)

Regards,
Timo

[Sir Gallantmon]

MSVC supports Intel syntax inline assembly, doesn't it? Why not use GAS with
Intel syntax mode, so that inline assembly would be compatible with both?

On Sat, Feb 6, 2010 at 9:41 AM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:

>  
>  No doubt the new code is a huge advantage over the old code. Still there
>  are some issues in my opinion.
>
>  Currently the trap handlers are generated inside C code by a macro
>  called KiTrap(), located in trap_x.h.
>  This macro creates a C function, that uses KiTrapStub inline function to
>  create the actual entry stub. KiTrapStub is 90% pure gcc inline assembly.
>  At the end it does a jmp to the real C handler.
>
>  Most C handlers will start processing with an inline function
>  KiEnterTrap, which is C code with inline assembly functions like
>  KiSetSaneSegments, Ke386GetFs. KiTrap06Handler/Trap0DHandler are
>  exceptions, they use KiIsV8086TrapSafe to determine in inline assembly
>  if we have a v86 trap.
>  In this case KiEnterV86Trap is used instead of KiEnterTrap. KiTrap02 is
>  also an exception, it's a pure C handler
>
>
>  Now what's wrong?
>
>  1.) The gcc inline assembly is very compiler specific and MSVC
>  unfriendly. The old method using GAS specific asm files was at least
>  linkable with MSVC code. The new code needs porting.
>
>  2.) The current C code makes assumptions about what the compiler will
>  do, ignoring the ABI, which requires that you do not change ds/es in C
>  code, not even temporarily and that the direction flag is clear on
>  function entry. The compiler might at any place use ds/es for whatever
>  it likes. It must be a valid flat segment. Only using stack variables
>  might work on the current gcc, but might fail on other compilers. A good
>  example would be MSVC's use of __security_cookie, which is referenced on
>  function entry through ds. I remember that not having cleared the
>  direction flag in a trap handler caused strange random bugs in the past,
>  we should avoid making this mistake again. KiTrapStub even changes esp
>  inside C code (in cases where the function is entered without valid esp)
>
>
>  Suggested modifications:
>
>  Implement the trap entry functions in a pure assembly file, using
>  assembly macros which would combine KiTrapStub and the segment handling
>  part of KiEnterTrap.
>  Trap exit could possibly be done this way as well. On the amd64 branch
>  the C handlers return a status value back to the assembly stub, but a
>  jump or call could be done as well.
>  It would be possible to use ML for these assembly files as well. This
>  works fine in the amd64 branch.
>  Also strictly stick to C ABI instead of using compiler extensions and
>  making assumptions about how the compiler works.
>  It would improve portability, make the code safe and also more readable.
>  And we would not really add more assembly, just move the current inline
>  assembly to a pure assembly file.
>  Possible performance disadvantages: I don't see any. We might end up
>  with a jmp or call instruction more, but we can possibly safe this
>  somewhere else.
>
>  And maybe finally someone deletes the disgusting Ke386SetDs()... ;-)
>
>  Regards,
>  Timo
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>