[ros-dev] msvc build update

[Ros Arm]

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 at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev