[syslinux] [PATCH 3/5] installers: MSVC compatibility fixes

[Gene Cumm]

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