Re: [ros-dev] [ros-diffs] [jgardou] 53547: [VGA_NEW] - fix some warnings

[Alex Ionescu]

These changes are wrong :(

Please use the correct compiler flag instead (-mrtd)

Best regards,
Alex Ionescu

On Sat, Sep 3, 2011 at 5:09 PM, [e-mail] wrote:
> Author: jgardou
> Date: Sat Sep  3 16:09:34 2011
> New Revision: 53547
>
> URL: http://svn.reactos.org/svn/reactos?rev=53547&view=rev
> Log:
> [VGA_NEW]
> fix some warnings
>
> Modified:
>    trunk/reactos/drivers/video/miniport/vga_new/vga.c
>
> Modified: trunk/reactos/drivers/video/miniport/vga_new/vga.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/video/miniport/vga_...
>
> ==============================================================================
> --- trunk/reactos/drivers/video/miniport/vga_new/vga.c [iso-8859-1]
> (original)
> +++ trunk/reactos/drivers/video/miniport/vga_new/vga.c [iso-8859-1] Sat Sep
>  3 16:09:34 2011
> @@ -19,6 +19,7 @@
>  //
>
> VP_STATUS
> +NTAPI
>  VgaFindAdapter(
>     PVOID HwDeviceExtension,
>     PVOID HwContext,
> @@ -28,11 +29,13 @@
>     );
>
> BOOLEAN
> +NTAPI
>  VgaInitialize(
>     PVOID HwDeviceExtension
>     );
>
> BOOLEAN
> +NTAPI
>  VgaStartIO(
>     PVOID HwDeviceExtension,
>     PVIDEO_REQUEST_PACKET RequestPacket
> @@ -102,6 +105,7 @@
>     );
>
> VP_STATUS
> +NTAPI
>  GetDeviceDataCallback(
>    PVOID HwDeviceExtension,
>    PVOID Context,
> @@ -130,7 +134,6 @@
>  #pragma alloc_text(PAGE,VgaSetColorLookup)
>  #endif
>
> -
>
> //---------------------------------------------------------------------------
>  ULONG
>  // eVb: 1.3 [GCC] - Add NTAPI for GCC support
> @@ -303,9 +306,10 @@
>     return initializationStatus;
>
> } // end DriverEntry()
> -
>
> //---------------------------------------------------------------------------
>  VP_STATUS
> +NTAPI
>  VgaFindAdapter(
>     PVOID HwDeviceExtension,
>     PVOID HwContext,
> @@ -383,7 +387,7 @@
>     if ((ConfigInfo->AdapterInterfaceType == Internal) &&
>         (VideoPortGetDeviceData(HwDeviceExtension,
>                                 VpControllerData,
>                            &GetDeviceDataCallback,
>                            GetDeviceDataCallback,
>                           VgaAccessRange) != NO_ERROR))
> {
>       return ERROR_INVALID_PARAMETER;
>
> @@ -485,9 +489,10 @@
>
> } // VgaFindAdapter()
> -
>
> //---------------------------------------------------------------------------
>  BOOLEAN
> +NTAPI
>  VgaInitialize(
>     PVOID HwDeviceExtension
>     )
> @@ -527,9 +532,10 @@
>     return TRUE;
>
> } // VgaInitialize()
> -
>
> //---------------------------------------------------------------------------
>  BOOLEAN
> +NTAPI
>  VgaStartIO(
>     PVOID HwDeviceExtension,
>     PVIDEO_REQUEST_PACKET RequestPacket
> @@ -1440,8 +1446,9 @@
>     return ERROR_INVALID_PARAMETER;
>
> } // end VgaSetColorLookup()
> -
>
> VP_STATUS
> +NTAPI
>  GetDeviceDataCallback(
>     PVOID HwDeviceExtension,
>     PVOID Context,

[Jérôme Gardou]

Yuou're right... It was a bad mistake in my local copy which caused the 
warnings to appear.

Thanks for pointing this out.
BTW, I took a look at it, and our headers have the NTAPI calling 
convention, whereas WDK ones haven't.

Le 03/09/2011 18:21, Alex Ionescu a écrit :
> These changes are wrong :(
>
> Please use the correct compiler flag instead (-mrtd)
>
> Best regards,
> Alex Ionescu

[Timo Kreuzer]

Am 03.09.2011 18:21, schrieb Alex Ionescu:
> These changes are wrong :(
>
> Please use the correct compiler flag instead (-mrtd)
>
> Best regards,
> Alex Ionescu

Can you explain what exactly is "wrong" with this change and why we use 
-mrtd here and in a few other places?
Is there anything "better" in using this (IMO hack) instead of using 
proper definitions, like we have elsewhere?

Oh, btw, -mrtd conflicts with some of out crt headers

Regards,
Timo

[Dmitry Gorbachev]

BTW, -mrtd has recently (in April) changed its behavior in GCC trunk.
If reactos needs this flag, then maybe somebody should ask the author
of that change (Kai) to provide another flag (say, -mold-rtd) instead
of it.

[Jérôme Gardou]

May I ask what the change is?

Le 03/09/2011 22:53, Dmitry Gorbachev a écrit :
> BTW, -mrtd has recently (in April) changed its behavior in GCC trunk.
> If reactos needs this flag, then maybe somebody should ask the author
> of that change (Kai) to provide another flag (say, -mold-rtd) instead
> of it.
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Dmitry Gorbachev]

> May I ask what the change is?

With -mrtd, it now does not add @n suffix to stdcall functions, and
also assumes that libgcc functions are stdcall (that they pop
arguments).

[Alex Ionescu]

That's always been the case, and easily fixable with the switch I mentioned
which disables libgcc built-in functions (which shouldn't be used anyway).

For exported functions, I think MS always uses explicit convention in their
headers and sample source, so it shouldn't be a problem.

Best regards,
Alex Ionescu

On Sat, Sep 3, 2011 at 11:32 PM, Dmitry Gorbachev [e-mail]:
> > May I ask what the change is?
>
> With -mrtd, it now does not add @n suffix to stdcall functions, and
> also assumes that libgcc functions are stdcall (that they pop
> arguments).
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Dmitry Gorbachev]

> That's always been the case,

At least in 4.4, 4.5, and 4.6, as I see, libgcc functions are assumed
to be cdecl even with -mrtd.
> and easily fixable with the switch I mentioned
> which disables libgcc built-in functions (which shouldn't be used anyway).

I don't know any GCC switch which disables calls to libgcc functions.
These are used, for example, to implement long long integer division
(happens in classpnp, and it's compiled with -mrtd).

-fno-builtin, it disables built-in functions such as cos or strlen.
> For exported functions, I think MS always uses explicit convention in their
> headers and sample source, so it shouldn't be a problem.

But now with -mrtd, the suffix is not added even to functions
explicitly declared as stdcall.

[Dmitry Gorbachev]

> At least in 4.4, 4.5, and 4.6, as I see, libgcc functions are assumed
> to be cdecl even with -mrtd.

I was wrong about 4.7; it works well, too.
> But now with -mrtd, the suffix is not added even to functions
> explicitly declared as stdcall.

This observation is still valid.

[Alex Ionescu]

For the export issue, can this be hacked around in the def or with a linker
flag to force @ (--force-at)?

Cause this sounds like a bug -- maybe talk to Kai? Amine?

-mrtd is really useful as more MS/3rd-party WDK-compatible code gets merged
in....

Best regards,
Alex Ionescu

On Sun, Sep 4, 2011 at 2:45 AM, Dmitry Gorbachev [e-mail]:
> > At least in 4.4, 4.5, and 4.6, as I see, libgcc functions are assumed
> > to be cdecl even with -mrtd.
>
> I was wrong about 4.7; it works well, too.
> > But now with -mrtd, the suffix is not added even to functions
> > explicitly declared as stdcall.
>
> This observation is still valid.
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Dmitry Gorbachev]

> For the export issue, can this be hacked around in the def or with a linker
> flag to force @ (--force-at)?

There is a flag --kill-at, but no --force-at. I tried
--enable-stdcall-fixup, but for some reason it does not help.

[Alex Ionescu]

Hey Timo,

I replied to another e-mail why this should be done.

Also, -fno-built-in-function-warnings or something like that fixes the CRT
headers.

Best regards,
Alex Ionescu

On Sat, Sep 3, 2011 at 7:00 PM, Timo Kreuzer [e-mail] wrote:
> Am 03.09.2011 18:21, schrieb Alex Ionescu:
>
> These changes are wrong :(
> > Please use the correct compiler flag instead (-mrtd)
> >
> > Best regards,
> > Alex Ionescu
>
> Can you explain what exactly is "wrong" with this change and why we use
> -mrtd here and in a few other places?
> Is there anything "better" in using this (IMO hack) instead of using proper
> definitions, like we have elsewhere?
>
> Oh, btw, -mrtd conflicts with some of out crt headers
>
> Regards,
> Timo
>
> ______________________________**_________________
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/**mailman/listinfo/ros-devhttp://www.reactos.org/mailman/listinfo/ros-dev