Re: [ros-dev] [ros-diffs] [ekohl] 53469: [WIDL] Revert parts of r53171: Remove the -Oif flag for WIDL because the NT4 stub code generated by the current WIDL is heavily broken with respect to the range attribute and others....

[Jérôme Gardou]

HI Eric!

I'm not sure about what you mean for range attribute, but I notice that
typedef [context_handle] PVOID my_handle;
makes widl thinks that any value of the my_handle type is passed by 
pointer, whereas midl considers it is passed by value.

Could you be more precise about "range attributes and others?" please.

Regards.
Jérôme.

Le 27/08/2011 18:24, [e-mail] a écrit :
> Author: ekohl
> Date: Sat Aug 27 16:24:40 2011
> New Revision: 53469
>
> URL: http://svn.reactos.org/svn/reactos?rev=53469&view=rev
> Log:
> [WIDL]
> Revert parts of r53171:
> Remove the -Oif flag for WIDL because the NT4 stub code generated by the current WIDL is heavily broken with respect to the range attribute and others. Please enable the -Oif feature only after these bugs are fixed in WIDL!
>
> Modified:
>      trunk/reactos/cmake/idl-support.cmake
>      trunk/reactos/tools/rbuild/backend/mingw/rules.mak
>
> Modified: trunk/reactos/cmake/idl-support.cmake
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/cmake/idl-support.cmake?rev...
> ==============================================================================
> --- trunk/reactos/cmake/idl-support.cmake [iso-8859-1] (original)
> +++ trunk/reactos/cmake/idl-support.cmake [iso-8859-1] Sat Aug 27 16:24:40 2011
> @@ -22,8 +22,8 @@
>       set(IDL_HEADER_ARG -h -o) #.h
>       set(IDL_HEADER_ARG2 -h -H) #.h
>       set(IDL_TYPELIB_ARG -t -o) #.tlb
> set(IDL_SERVER_ARG -Oif -s -o) #.c for server library
> set(IDL_CLIENT_ARG -Oif -c -o) #.c for stub client library
> set(IDL_SERVER_ARG -s -o) #.c for server library
> set(IDL_CLIENT_ARG -c -o) #.c for stub client library
>  set(IDL_PROXY_ARG -p -o)
>  set(IDL_INTERFACE_ARG -u -o)
>  if(ARCH MATCHES i386)
>
> @@ -169,6 +169,6 @@
>           list(APPEND IID_SOURCES ${NAME}_i.c)
>       endforeach()
>       add_library(${TARGET} ${IID_SOURCES})
> add_dependencies(${TARGET} psdk)
> add_dependencies(${TARGET} psdk)
>  set_target_properties(${TARGET} PROPERTIES EXCLUDE_FROM_ALL TRUE)
> endfunction()
>
> Modified: trunk/reactos/tools/rbuild/backend/mingw/rules.mak
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/tools/rbuild/backend/mingw/...
> ==============================================================================
> --- trunk/reactos/tools/rbuild/backend/mingw/rules.mak [iso-8859-1] (original)
> +++ trunk/reactos/tools/rbuild/backend/mingw/rules.mak [iso-8859-1] Sat Aug 27 16:24:40 2011
> @@ -264,7 +264,7 @@
>
> ${call RBUILD_intermediate_path_noext,$(2)}_c: $(2) $(3) $$(widl_TARGET) | ${call RBUILD_intermediate_dir,$(2)}
>    $$(ECHO_WIDL)
> $$(Q)$$(widl_TARGET) ${call RBUILD_midlflags,$(1),$(4),-I${call RBUILD_dir,$(2)}} -Oif -h -H ${call RBUILD_intermediate_path_noext,$(2)}_c.h -c -C ${call RBUILD_intermediate_path_noext,$(2)}_c.c $(2)
>
> $$(Q)$$(widl_TARGET) ${call RBUILD_midlflags,$(1),$(4),-I${call RBUILD_dir,$(2)}} -h -H ${call RBUILD_intermediate_path_noext,$(2)}_c.h -c -C ${call RBUILD_intermediate_path_noext,$(2)}_c.c $(2)
> $${checkpoint} $$@>$(NUL)
>
> ${call RBUILD_CC,$(1),${call RBUILD_intermediate_path_noext,$(2)}_c.c,,,${call RBUILD_intermediate_path_noext,$(2)}_c.o}
>
> @@ -280,7 +280,7 @@
>
> ${call RBUILD_intermediate_path_noext,$(2)}_s: $(2) $(3) $$(widl_TARGET) | ${call RBUILD_intermediate_dir,$(2)}
>    $$(ECHO_WIDL)
> $$(Q)$$(widl_TARGET) ${call RBUILD_midlflags,$(1),$(4),-I${call RBUILD_dir,$(2)}} -Oif -h -H ${call RBUILD_intermediate_path_noext,$(2)}_s.h -s -S ${call RBUILD_intermediate_path_noext,$(2)}_s.c $(2)
>
> $$(Q)$$(widl_TARGET) ${call RBUILD_midlflags,$(1),$(4),-I${call RBUILD_dir,$(2)}} -h -H ${call RBUILD_intermediate_path_noext,$(2)}_s.h -s -S ${call RBUILD_intermediate_path_noext,$(2)}_s.c $(2)
> $${checkpoint} $$@>$(NUL)
>
> ${call RBUILD_CC,$(1),${call RBUILD_intermediate_path_noext,$(2)}_s.c,,,${call RBUILD_intermediate_path_noext,$(2)}_s.o}

This commit also breaks up opengl32 winetest:
http://www.reactos.org/testman/compare.php?ids=7618,7621,7622

On Sat, 27 Aug 2011 23:11 +0200, "Jérôme Gardou" [e-mail] wrote:
> HI Eric!
>
> I'm not sure about what you mean for range attribute, but I notice that
> typedef [context_handle] PVOID my_handle;
> makes widl thinks that any value of the my_handle type is passed by 
> pointer, whereas midl considers it is passed by value.
>
> Could you be more precise about "range attributes and others?" please.
>
> Regards.
> Jérôme.

With best regards
Caemyr

[Eric Kohl]

> This commit also breaks up opengl32 winetest:
> http://www.reactos.org/testman/compare.php?ids=7618,7621,7622

I guess you will see an entirely different test result if you only run 
the opengl32 test right after booting ReactOS.

IMO these 'floating' test results show that there are bugs buried deep 
in the system. Memory corruptions come to my mind...

Regards
Eric

Indeed, the crash looks intermittent.

On Sun, 28 Aug 2011 00:42 +0200, "Eric Kohl" [e-mail] wrote:
> > This commit also breaks up opengl32 winetest:
> > http://www.reactos.org/testman/compare.php?ids=7618,7621,7622
>
> I guess you will see an entirely different test result if you only run 
> the opengl32 test right after booting ReactOS.
>
> IMO these 'floating' test results show that there are bugs buried deep 
> in the system. Memory corruptions come to my mind...
>
> Regards
> Eric

With best regards
Caemyr

[Eric Kohl]

Hello Jérôme!
> I'm not sure about what you mean for range attribute, but I notice that
> typedef [context_handle] PVOID my_handle;
> makes widl thinks that any value of the my_handle type is passed by
> pointer, whereas midl considers it is passed by value.

Did you try to find the bug in WIDL?
> Could you be more precise about "range attributes and others?" please.

I should better have written "range attributes and probably others"!

The range attribute is used in the svcctl.idl file and enables range 
checks in rpcrt4.dll. This is a nice feature, but WIDL is messing up the 
ranges if a range attribute is used with function parameters.

For example:
DWORD Test(
     [in] SC_RPC_HANDLE hSCManager,
     [in, range(0, 1024)] DWORD dwParam1,
     [in, range(0, 255)] DWORD dwParam2);

WIDL will emit the same range type (range 0-255) for both parameters 
because the range attribute is assigned to the DWORD type and it is 
overwritten whenever a new range attribute is used with the DWORD type.
In the end all DWORD type parameters that have a range attribute will 
use the range that is used by the last DWORD type with a range attribute 
(dwParam2 in this case) in the idl file. This will cause some 'funny' 
effects if dwParam1 is greater than 255. :-/

BTW, I think it is more important to have a working RPC system in 
ReactOS than a windows-compatible one. The compatibility issues can be 
fixed later, but if you break RPC today by fixing compatibility issues 
you will render ReactOS unusable. So please be very careful when you 
make changes to rpcrt4 or WIDL.

Regards,
Eric

[Jérôme Gardou]

Please see inlined answers.

Le 28/08/2011 00:29, Eric Kohl a écrit :
> Hello Jérôme!
> > I'm not sure about what you mean for range attribute, but I notice that
> > typedef [context_handle] PVOID my_handle;
> > makes widl thinks that any value of the my_handle type is passed by
> > pointer, whereas midl considers it is passed by value.
>
> Did you try to find the bug in WIDL?

In fact, it was our bug, fixed in r53477.
> > Could you be more precise about "range attributes and others?" please.
>
> I should better have written "range attributes and probably others"!
>
> The range attribute is used in the svcctl.idl file and enables range 
> checks in rpcrt4.dll. This is a nice feature, but WIDL is messing up 
> the ranges if a range attribute is used with function parameters.
>
> For example:
> DWORD Test(
>     [in] SC_RPC_HANDLE hSCManager,
>     [in, range(0, 1024)] DWORD dwParam1,
>     [in, range(0, 255)] DWORD dwParam2);
>
> WIDL will emit the same range type (range 0-255) for both parameters 
> because the range attribute is assigned to the DWORD type and it is 
> overwritten whenever a new range attribute is used with the DWORD type.
> In the end all DWORD type parameters that have a range attribute will 
> use the range that is used by the last DWORD type with a range 
> attribute (dwParam2 in this case) in the idl file. This will cause 
> some 'funny' effects if dwParam1 is greater than 255. :-/

OK, I guess we'd have to open a bug on wine side, I'm not comfortable 
enough with WIDL code to fix it myself.
> BTW, I think it is more important to have a working RPC system in 
> ReactOS than a windows-compatible one. The compatibility issues can be 
> fixed later, but if you break RPC today by fixing compatibility issues 
> you will render ReactOS unusable. So please be very careful when you 
> make changes to rpcrt4 or WIDL.

I fully agree with that, but just try to reenable -Oif, and look at 
advapi32:service winetest results. You'd be surprised : no skipped 
tests, and some others are actually fixed! (while others are broken, 
most probably because of this range thing) So it's more than a "nice 
feature". You can already see it fixes one eventlog test by looking at 
the link Olaf provided (look for eventlog.c:206).
> Regards,
> Eric
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

Regards.
Jérôme.