Syslinux - Mar 2016 - [PATCH 3/5] installers: MSVC compatibility fixes

Hi Shao,

You're right, "a=b=<immediate value>;" wasn't the actual
issue.

On 2016.03.06 20:34, Shao Miller via Syslinux wrote:
> If this change is simply due to a mental note about an incident where a
> compiler once complained about this type of thing
The problem was due to the following warning when compiling for 64-bit 
using using the latest WDK (7600.16385.1), with warning level 3 (/W3):

1>c:\rufus\src\syslinux\libinstaller\syslxmod.c(44) : warning C4242:
'='
: conversion from 'sector_t' to 'unsigned int', possible loss of
data

I used to compile Rufus with /W3 everywhere when using WDK, but I have 
very recently reduced the warning level to /W2 for the Syslinux part 
(see [1]) , so that I could align with your source without having to 
also apply a bunch of extra patches due to WDK compilation breakage 
without additional casts (e.g. [2], [3], bearing in mind that this is a 
commit that removes those casts). I remember that you guys weren't too 
receptive about adding those casts to the source, when I proposed them 4 
years ago [4]...

With /W2, you are correct that WDK doesn't complain about this specific 
line any more, so I can live with this part not being applied. That is, 
unless you also want to consider people who may be compiling part of the 
Syslinux source using WDK with /W3 (which I wouldn't mind being able to 
reinstate for Rufus compilation, but in that case, Syslinux would also 
need these additional cast patches).

Regards,

/Pete

[1] 
https://github.com/pbatard/rufus/commit/db0880e534e4e9504e8c283256fbfbd0683abded#diff-729493b04f2117d4290873c20e986726
[2] 
https://github.com/pbatard/rufus/commit/db0880e534e4e9504e8c283256fbfbd0683abded#diff-9b695506cac9f006f20d03d23ee289e0
[3] 
https://github.com/pbatard/rufus/commit/db0880e534e4e9504e8c283256fbfbd0683abded#diff-55f6a1a381f24361820e0047aec04222
[4] http://www.syslinux.org/archives/2012-February/017643.html

On 3/6/2016 17:03, Pete Batard via Syslinux wrote:
> The problem was due to the following warning when compiling for 64-bit 
> using using the latest WDK (7600.16385.1), with warning level 3 (/W3):
>
> 1>c:\rufus\src\syslinux\libinstaller\syslxmod.c(44) : warning C4242: 
> '=' : conversion from 'sector_t' to 'unsigned int',
possible loss of data
Then your patch seems like the most sensible solution, as this warning 
identifies that the two objects have different types.  That declaration 
context wasn't included in the patch; too far up. :)
> I used to compile Rufus with /W3 everywhere when using WDK, but I have 
> very recently reduced the warning level to /W2 for the Syslinux part 
> (see [1]) , so that I could align with your source without having to 
> also apply a bunch of extra patches due to WDK compilation breakage 
> without additional casts (e.g. [2], [3], bearing in mind that this is 
> a commit that removes those casts). I remember that you guys weren't 
> too receptive about adding those casts to the source, when I proposed 
> them 4 years ago [4]...
/W3 and other strict modes of translation in Microsoft's and others' C 
implementations are enjoyable, except when they're not.  I agree that 
it'd be nice to translate in every [worthy and] targeted implementation 
without incident.

Regarding [4]: The code expects that the shifted 'libfat_sector_t' value
will certainly fit in the 31 value bits of an 'int32_t'.  The logic 
prior to that might even prove it, but we'd have to follow such things 
as whether or not 'fs->clustshift' is sanity-checked, etc.

If the warning is ugly and the cast is ugly, there are other solutions, 
including:

1. Increase the precision of 'cluster'.  It seems that there are bitwise
operations performed, so this probably isn't a good idea.

2. Use a macro or inline function with a descriptive name to deal with 
the conversion.

3. Disable the Microsoft precision warning, since Syslinux does lots of 
this sort of thing.

That last option seems pretty simple, without moving entirely from /W3 
to /W2.  Could that work?  (C4242)
> With /W2, you are correct that WDK doesn't complain about this 
> specific line any more, so I can live with this part not being 
> applied. That is, unless you also want to consider people who may be 
> compiling part of the Syslinux source using WDK with /W3 (which I 
> wouldn't mind being able to reinstate for Rufus compilation, but in 
> that case, Syslinux would also need these additional cast patches).
Your separate-line-for-each patch seems like the most sensible solution.

--
Shao Miller
https://twitter.com/synthetel

On Sun, Mar 6, 2016 at 8:21 PM, Shao Miller via Syslinux
<syslinux at zytor.com> wrote:
> On 3/6/2016 17:03, Pete Batard via Syslinux wrote:
>>
>> The problem was due to the following warning when compiling for 64-bit
>> using using the latest WDK (7600.16385.1), with warning level 3 (/W3):
>>
>> 1>c:\rufus\src\syslinux\libinstaller\syslxmod.c(44) : warning C4242:
'=' :
>> conversion from 'sector_t' to 'unsigned int', possible
loss of data
>
>
> Then your patch seems like the most sensible solution, as this warning
> identifies that the two objects have different types.  That declaration
> context wasn't included in the patch; too far up. :)
Add to this that you need to hunt other source files to find that the
first is signed.

I'd say this changeset needs to be split.  The first is about packing
compatibility.


libinstaller: PACKED compatibility for MSVC

MSVC requires prefix and suffix attributes for packing structures.


while the second is all about casting:


libinstaller: remove signed to unsigned cast in initialization
>> I used to compile Rufus with /W3 everywhere when using WDK, but I have
>> very recently reduced the warning level to /W2 for the Syslinux part
(see
>> [1]) , so that I could align with your source without having to also
apply a
>> bunch of extra patches due to WDK compilation breakage without
additional
>> casts (e.g. [2], [3], bearing in mind that this is a commit that
removes
>> those casts). I remember that you guys weren't too receptive about
adding
>> those casts to the source, when I proposed them 4 years ago [4]...
>
>
> /W3 and other strict modes of translation in Microsoft's and
others' C
> implementations are enjoyable, except when they're not.  I agree that
it'd
> be nice to translate in every [worthy and] targeted implementation without
> incident.
>
> Regarding [4]: The code expects that the shifted 'libfat_sector_t'
value
> will certainly fit in the 31 value bits of an 'int32_t'.  The logic
prior to
> that might even prove it, but we'd have to follow such things as
whether or
> not 'fs->clustshift' is sanity-checked, etc.
>
> If the warning is ugly and the cast is ugly, there are other solutions,
> including:
>
> 1. Increase the precision of 'cluster'.  It seems that there are
bitwise
> operations performed, so this probably isn't a good idea.
>
> 2. Use a macro or inline function with a descriptive name to deal with the
> conversion.
>
> 3. Disable the Microsoft precision warning, since Syslinux does lots of
this
> sort of thing.
>
> That last option seems pretty simple, without moving entirely from /W3 to
> /W2.  Could that work?  (C4242)
>
>> With /W2, you are correct that WDK doesn't complain about this
specific
>> line any more, so I can live with this part not being applied. That is,
>> unless you also want to consider people who may be compiling part of
the
>> Syslinux source using WDK with /W3 (which I wouldn't mind being
able to
>> reinstate for Rufus compilation, but in that case, Syslinux would also
need
>> these additional cast patches).
>
>
> Your separate-line-for-each patch seems like the most sensible solution.
Some of the warnings do make sense while some others seem to be thrown
for more pedantic reasons that are not issues when some individual
code is examined.

The Syslinux core, COM32 modules, and MEMDISK are dominantly built
with a more relaxed warning set.  Since most of that was in place
prior to my involvement, my sole suspicions are that it's related to
the lower protection mode and lack of a typical kernel's memory
protection mechanisms.

For the installers and other tools for normal OSs, they should be
built with the inherited warning set.  Fixes such as removing this
cast allow the installers to be more portably rebuilt for other
platforms.  Within the last year or so, I've now heard of people
building installers for a *BSD (not sure which of
FreeBSD/OpenBSD/NetBSD) and now MS Windows with MS Visual Studio.  I
feel that reasonably facilitating portability like this helps the
overall community to allow the tools to be used in more ways that suit
eventual users.

-- 
-Gene

On 2016.03.07 01:21, Shao Miller via Syslinux wrote:
> 3. Disable the Microsoft precision warning, since Syslinux does lots of
> this sort of thing.
>
> That last option seems pretty simple, without moving entirely from /W3
> to /W2.  Could that work?  (C4242)
Unfortunately, there are some warnings that Microsoft, in their great 
wisdom, decided that driver developers should not choose to ignore, and 
this is one of them...

/wd4242 (which is how you disable warnings in WDK), is one of the things 
I tried before going /W2, only to find that, unlike other warnings, 
which I double-checked you can actually silence with /wd (for instance 
/wd4018 --signed/unsigned mismatch-- can be disabled alright), this one 
could not be silenced.

Regards,

/Pete