Re: [ros-dev] [ros-diffs] [jgardou] 54462: [RTL] - explicitly truncate some values, so it doesn't count as an error for compiler/MSVC runtime checker

[Timo Kreuzer]

That doesn't make much sense.
Whats the point of the LOWORD() macro, if not truncating it ?
Fix the macro if its broken.


Am 20.11.2011 20:45, schrieb jgardou(a)svn.reactos.org:
>   Author: jgardou
>  Date: Sun Nov 20 19:45:06 2011
>  New Revision: 54462
>
>  URL: http://svn.reactos.org/svn/reactos?rev=54462&view=rev
>  Log:
>  [RTL]
>  - explicitly truncate some values, so it doesn't count as an error for compiler/MSVC
> runtime checker
>
>  Modified:
>       trunk/reactos/lib/rtl/image.c
>
>  Modified: trunk/reactos/lib/rtl/image.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/rtl/image.c?rev=54462&…
>  ==============================================================================
>  --- trunk/reactos/lib/rtl/image.c [iso-8859-1] (original)
>  +++ trunk/reactos/lib/rtl/image.c [iso-8859-1] Sun Nov 20 19:45:06 2011
>  @@ -369,7 +369,7 @@
>    {
>        SHORT Offset;
>        USHORT Type;
>  -    USHORT i;
>  +    ULONG i;
>        PUSHORT ShortPtr;
>        PULONG LongPtr;
>        PULONGLONG LongLongPtr;
>  @@ -379,7 +379,6 @@
>            Offset = SWAPW(*TypeOffset)&  0xFFF;
>            Type = SWAPW(*TypeOffset)>>  12;
>            ShortPtr = (PUSHORT)(RVA(Address, Offset));
>  -
>            /*
>            * Don't relocate within the relocation section itself.
>            * GCC/LD generates sometimes relocation records for the relocation section.
>  @@ -398,16 +397,16 @@
>                break;
>
>            case IMAGE_REL_BASED_HIGH:
>  -            *ShortPtr = HIWORD(MAKELONG(0, *ShortPtr) + (LONG)Delta);
>  +            *ShortPtr = HIWORD(MAKELONG(0, *ShortPtr) + (Delta&  0xFFFFFFFF));
>                break;
>
>            case IMAGE_REL_BASED_LOW:
>  -            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta);
>  +            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta&  0xFFFF);
>                break;
>
>            case IMAGE_REL_BASED_HIGHLOW:
>                LongPtr = (PULONG)RVA(Address, Offset);
>  -            *LongPtr = SWAPD(*LongPtr) + (ULONG)Delta;
>  +            *LongPtr = SWAPD(*LongPtr) + (Delta&  0xFFFFFFFF);
>                break;
>
>            case IMAGE_REL_BASED_DIR64:
>
>
>

[Jérôme Gardou]

Here the variable is an ULONGLONG and we pass it to a macro that casts 
it to a DWORD, and that's what it should do. Although casting and 
truncating is the same from a "normal" compiler point of view, a run 
time checker would raise a warning if you cast a value superior to 
0xFFFFFFFF to a DWORD. Truncating it explicitly tells it "yes, I 
truncate this value, not for avoiding compiler warning, but because 
that's what I want". See r54460 to see which kind of bugs such checkers 
could help to fix.

See also http://msdn.microsoft.com/en-us/magazine/cc301374.aspx for some 
in depth details, and, of course, cl documentation.
Also, gcc has this kind of functionalities, it's called GNAT.

Mit freundlichen Grüssen. :-)
Jérôme

Le 24/11/2011 00:03, Timo Kreuzer a écrit :
>   That doesn't make much sense.
>  Whats the point of the LOWORD() macro, if not truncating it ?
>  Fix the macro if its broken.
>
>
>  Am 20.11.2011 20:45, schrieb jgardou(a)svn.reactos.org:
> >   Author: jgardou
> >  Date: Sun Nov 20 19:45:06 2011
> >  New Revision: 54462
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=54462&view=rev
> >  Log:
> >  [RTL]
> >  - explicitly truncate some values, so it doesn't count as an error 
> >  for compiler/MSVC runtime checker
> >
> >  Modified:
> >       trunk/reactos/lib/rtl/image.c
> >
> >  Modified: trunk/reactos/lib/rtl/image.c
> >  URL: 
> >
> > http://svn.reactos.org/svn/reactos/trunk/reactos/lib/rtl/image.c?rev=54462&…
> >  ============================================================================== 
> >
> >  --- trunk/reactos/lib/rtl/image.c [iso-8859-1] (original)
> >  +++ trunk/reactos/lib/rtl/image.c [iso-8859-1] Sun Nov 20 19:45:06 2011
> >  @@ -369,7 +369,7 @@
> >    {
> >        SHORT Offset;
> >        USHORT Type;
> >  -    USHORT i;
> >  +    ULONG i;
> >        PUSHORT ShortPtr;
> >        PULONG LongPtr;
> >        PULONGLONG LongLongPtr;
> >  @@ -379,7 +379,6 @@
> >            Offset = SWAPW(*TypeOffset)&  0xFFF;
> >            Type = SWAPW(*TypeOffset)>>  12;
> >            ShortPtr = (PUSHORT)(RVA(Address, Offset));
> >  -
> >            /*
> >            * Don't relocate within the relocation section itself.
> >            * GCC/LD generates sometimes relocation records for the 
> >  relocation section.
> >  @@ -398,16 +397,16 @@
> >                break;
> >
> >            case IMAGE_REL_BASED_HIGH:
> >  -            *ShortPtr = HIWORD(MAKELONG(0, *ShortPtr) + (LONG)Delta);
> >  +            *ShortPtr = HIWORD(MAKELONG(0, *ShortPtr) + (Delta&  
> >  0xFFFFFFFF));
> >                break;
> >
> >            case IMAGE_REL_BASED_LOW:
> >  -            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta);
> >  +            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta&  0xFFFF);
> >                break;
> >
> >            case IMAGE_REL_BASED_HIGHLOW:
> >                LongPtr = (PULONG)RVA(Address, Offset);
> >  -            *LongPtr = SWAPD(*LongPtr) + (ULONG)Delta;
> >  +            *LongPtr = SWAPD(*LongPtr) + (Delta&  0xFFFFFFFF);
> >                break;
> >
> >            case IMAGE_REL_BASED_DIR64:
> >
> >
> >  
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Aleksey Bragin]

On 24.11.2011 3:27, Jérôme Gardou wrote:
>   -            *ShortPtr = SWAPW(*ShortPtr) +
> LOWORD(Delta);
>  +            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta&  0xFFFF);  
Maybe turn
off this "feature" of MSVC checker then? Excuse me, but it's 
literally a code pollution. Especially the above case, of firstly ANDing 
with 0xFFFF, and *then* applying LOWORD (which would not change anything 
at all).

Or, as Timo said, maybe fix the macro? Move the cast elsewhere, so at 
first you &, and only then cast?


WBR,
Aleskey.

[Jérôme Gardou]

Le 24/11/2011 09:04, Aleksey Bragin a écrit :
>   On 24.11.2011 3:27, Jérôme Gardou wrote:
> >   -            *ShortPtr = SWAPW(*ShortPtr) +
> > LOWORD(Delta);
> >  +            *ShortPtr = SWAPW(*ShortPtr) + LOWORD(Delta&  0xFFFF);  
>  Maybe
> turn off this "feature" of MSVC checker then? Excuse me, but 
>  it's literally a code pollution. Especially the above case, of firstly 
>  ANDing with 0xFFFF, and *then* applying LOWORD (which would not change 
>  anything at all).
>
>  Or, as Timo said, maybe fix the macro? Move the cast elsewhere, so at 
>  first you &, and only then cast?
>
>
>  WBR,
>  Aleskey.
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 
This one is a mistake on my side.

And the macro is correct. The right thing to do is & and then casting, 
as you say.
BTW, this feature is of by default, you have to turn it on with the /RTC 
flag.

[Timo Kreuzer]

Am 24.11.2011 00:27, schrieb Jérôme Gardou:
>   Truncating it explicitly tells it "yes, I
> truncate this value, not for 
>  avoiding compiler warning, but because that's what I want". See r54460 
>  to see which kind of bugs such checkers could help to fix.
>  
What's next?
(Value & 0xFFFF) -> warning because ANDing an ULONG64 is regarded as 
"silencing a warning" -> fix: 
__really_truncate_this_unsigned_int64_to_ushort_and_discard_lost_bits(Value); 
?

For me casting *is* already doing it "explicitly". And if someone 
doesn't know that casting is the same as truncating, he shouldn't do C 
coding.

I agree with Aleksey: turn this feature off, its a bit too much.

Timo