Hi Jose,
I think there are some misunderstandings regarding the GCC code base I would like to clear
up inline.
Hi,
Don't feel offended, the port to c is a very good thing and indeed better in
the most part than the previous asm implementation. I value your work very
much.
But there are some aspects badly implemented:
1) It is not possible to write a good stub in an inline function nor in a
regular macro due to c preprocessor limitations. Your implementation trust
the optimizer to remove constant conditionals, but it does not work as well
as you may think. Have you seen the generated code? It has a lot of
unnecessary branching.
I look at the generated code almost every build, so yes I have seen it. The optimizer is
working correctly and there are no unnecessary branches. All the branch checks in
functions such as KiExitTrap are removed and the correct inline code is generated in all
callers. If you are seeing a different result, something is wrong with your compiler or
build settings.
I have implemented it as an x-macro, where I can use
the preprocessor to select the options to be generated and it does not
generate any run time conditional branching.
That is another solution, however mine works just as well, as I can show with objdump.
2) You have added many inline assembly parts in
several files that should be
kept as portable as possible. These things are better put together in
architecture dependent files/directories. Furthermore, most are unnecessary,
my single trap stub in a single x-macro replaces most of your inlines.
I have merged all entry ASM code into one stub, but I have yet to merge the exit ASM Code
into one stub as well, so it is true there are too many similar functions for now. Yes,
trap_x needs to go to i386. But these are not "inefficiencies", it's the
result of a WIP.
3) Since the pointer to the trap frame is passed to
handlers, why do you
pass as extra parameters information that is already there? Access to
members of the KTRAP_FRAME is just as fast as access to local variables or
parameters, but saves on additional copies and allow more efficient register
usage.
If you are talking about the case where EFLAGS is being passed, there is a large comment
explaining why this is done. Perhaps you could try reading it, and understanding the
problems of segmentation even in a flat model.
I am not aware of any cases where extra parameters are being passed, other than for
specific inline helper functions, in which your argument does not exist due to the fact
the register usage has already been optimized by analyzing the inline and caller function,
so this is more efficient than re-reading the field from the trap frame.
Furthermore using regparam(3) for functions called
from inline
assembly, when actually only one param was needed, was really a very bad
idea.
You'll have to show an example, but I think you are once again misunderstanding inline
functions. Finally, I don't know if you are unaware of regparm(3), but using only one
parameter has no negative side-effects (if this is really the case). In this case, only
one register will be used. It is not as if the other 2 registers are "lost".
And you pass the same parameters to child handlers in
cascade, again
and again. This is very unefficient.
I have no idea what you are talking about, other than specific inline cases where there is
no "cascade" since the code is flat. Perhaps you'd care to have some
examples.
Today I simplified the syscall and
fastsyscall handlers removing a lot unnecessary parameter copying and
cascading, saving a lot of cycles and memory, while making it much easier to
read and understand.
If those are the handlers you are referring to, they are all inlined, making your argument
moot. There is no cascade or register passing.
4) Why did you resort to code patching to encode the
PKINTERRUPT parameter,
instead of generating a static variable named together with the stub, for
example? Wasn't it hackish? I hope you understand that you won't save even a
cycle that way, so why to use such an unportable and complex method?
This is the code that Windows implements as well and is documented to function as such in
multiple places. Also, I do not see how a "Static variable named together with the
stub" would work. No it was not hackish, it's an efficient design that ReactOS
must further emulate due to the fact it is documented and code depends on this model.
Again the comments explain this, if you wouldn't mind reading them.
The worst thing is the indiscriminate and spread use of inline assembly and
compiler specific features. Please think in placing such things in platform
specific includes and try to make them as general and reusable as possible,
instead of making the code base very difficult to port and maintain.
Perhaps you would like to go back to the 5000+ lines of GCC-only, i386-only assembly. Now
the only parts remaining with ASM code number less than 50 lines, while the rest of the
code is portable. In fact the whole point was to re-use a lot of this code in the ARM
port.
Please take this as constructive criticism and not as any personal offense.
We all can always learn better if we can accept criticism.
Likewise,
-r
Jose Catena
DIGIWAVES S.L.
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev