Sunset support for older platforms and gear

[Jeffrey Walton]

Hi Everyone,

I've avoided or side stepped this issue, but Jonathan Wakely brought it to light recently after looking at algparam and observing:

> AssignFromHelperClass binds a
> reference to NULL just to default-construct a temporary of some type, then
> binds a const-reference to that temporary, then casts away const to modify
> the value of that temporary. What seems to be a "VC60 workaround" introduces
> undefined behaviour by trying to be far too clever.

Jonathan's observations highlight some of the issues with supporting older platforms and gear. The older gear includes compilers like Microsoft's embedded eVC 4.0 compiler, and older IDEs like a 1990's era Code Warrior. I think the support was cutting edge back then, but its a hindrance or a detriment today.

My QUESTIONS are:

 (1) Where can we reasonably draw a line?
 (2) How should we proceed moving forward?
 (3) And how can we test that we are proceeding as agreed?

My personal feelings lie somewhere around:

 * Circa 2000's compilers and IDEs
 * C++03 support
 * GCC 4.2.1 due to OpenBSD

Note that the above does not say "No, we don't support C++11". We just have to maintain sources that are compatible with the project's goals and objectives. I've already started exploring this issue at "How to guard move constructors for C++03 and C++11?", http://stackoverflow.com/q/30797388.

Once we classify an item (like, say GCC 2.9 or 3.x; or Visual Studio 5.0 project files), what do we do with them. So my QUESTION here is:

  (4) What do we do with obsolete gear once we agree something is obsolete?

Jeff

[Jeffrey Walton]

Hi Robert,

On Tue, Jul 14, 2015 at 4:42 PM, Robert Roessler <rob...@rftp.com> wrote:
> First off, it is likely improper to address you directly... but as a member
> of the previous Crypto++ mailing list, I am unclear exactly how to post
> these days - would replying or just directly addressing
> cryptop...@googlegroups.com just work? :)

No problem, swinging it back to Crypto++ Users.

> Other than that, I have some time, and have been watching the the traffic
> for the last weeks - *someone* sure has a lot of energy! ;)

Yeah, I'm in between contracts, so I've been trying to clear as many
issues as possible :)

> But I think on this warning - which just started bugging me after I synced
> my WC yesterday after a lengthy gap - you were faked out like I was...

OK, the additional warnings on Visual Studio likely came because of
https://github.com/weidai11/cryptopp/commit/e67480dd9e8dcda57939fc7480e273b8d2e81335.

We also cleared some more misc.h warnings at
https://github.com/weidai11/cryptopp/commit/828c550389f79b035f4518a79b7138d281f0961c.

> believe the actual complaint from VS (2013 for me) is caused by the various
> low-level workhorse "rotate" functions declaring their "shift" param as an
> unsigned char - which is where the actual alleged truncation is taking place
> (or would).

Here's the commit/comment on the truncation to char:
https://github.com/weidai11/cryptopp/commit/457eaeaf2317faa30c86d6d3fc289f40c58cf177.
It added assert(y <= 255), which turned out to be spurious because of
assert(y <= sizeof(T)*8). The latter was really assert(y <= 32).

But I'm having trouble parsing what the problem is, or what you want me to do.

Where do you see a problem, or what do you suggest we do?

Jeff

[Jeffrey Walton]

On Tue, Jul 14, 2015 at 6:20 PM, Robert Roessler <rob...@rftp.com> wrote:
> Jeffrey Walton wrote:
>>
>> ...
>> OK, the additional warnings on Visual Studio likely came because of
>>
>> https://github.com/weidai11/cryptopp/commit/e67480dd9e8dcda57939fc7480e273b8d2e81335.
>>
>> We also cleared some more misc.h warnings at
>>
>> https://github.com/weidai11/cryptopp/commit/828c550389f79b035f4518a79b7138d281f0961c.
>>
>>> believe the actual complaint from VS (2013 for me) is caused by the
>>> various
>>> low-level workhorse "rotate" functions declaring their "shift" param as
>>> an
>>> unsigned char - which is where the actual alleged truncation is taking
>>> place
>>> (or would).
>>
>>
>> Here's the commit/comment on the truncation to char:
>>
>> https://github.com/weidai11/cryptopp/commit/457eaeaf2317faa30c86d6d3fc289f40c58cf177.
>> It added assert(y <= 255), which turned out to be spurious because of
>> assert(y <= sizeof(T)*8). The latter was really assert(y <= 32).
>>
>> But I'm having trouble parsing what the problem is, or what you want me to
>> do.
>>
>> Where do you see a problem, or what do you suggest we do?
>
>

> I obviously left out some details... like what exactly I was talking about.
> ;)
>
> I get the C4242 issues (in the MSVC conditional block between line 752 and
> line 823) on all the "shift and rotate" functions in misc.h because the
> underlying workhorse functions being called expect an "unsigned char" for
> the shift/rotate count param, but the wrapper functions themselves are
> specifying an "unsigned int" param... so when it is passed down to the inner
> funcs, it generates the warning.
>
> So, for these, just use whatever form of cast you like the best to say "hey,
> these are really unsigned chars" on the shift/rotate count param when you
> make the inner calls. :)

Here's one of the functions (from line 755):

template<> inline
word16 rotlFixed<word16>(word16 x, unsigned int y)
{
assert(y < 8*sizeof(x));
return static_cast<word16>(y ? _rotl16(x, y) : x);
}

OK, so there are two use cases here. First is the library calling it
(1). Second is external callers using it (2).

*If* only use case (1) existed, then we could cast to silence it. But
because of use case (2), we want callers to have the benefit of
analysis.

We can possibly do something like: find all library callers (use case
(1)), and ensure they don't trigger the warning. Then, any warnings
that remain are user's code (use case (2)).

I think we really need a rotate that operates on a data type larger
than a byte. We can possibly do that with inline assembly on x86. But
we won't really have a solution for x64 unless we provide MASM64
routines in a stand alone source file (x64 lacks inline assembly).

> Further, I might be inclined to also roll back the commit that added all the
> static_cast<word16> stuff to this block - at least if it was motivated by
> thinking that was the cause of the C4242 warnings - since the type being
> returned by the wrapped functions is already compatible with the the types
> returned by the wrapper functions.

I think that was a different complaint. I think we are getting them
piecemeal because of the 'pragam once'. The idea was to avoid flooding
the code with all the warnings.

I'll verify it once I get a Windows VM fired up.

Jeff

[Robert Roessler]

Jeffrey Walton wrote:

I believe we are still not communicating...

I will suggest there is no "case 1/2" - this is ALL internal to these templated "wrapper" functions - at least in my opinion.  The issue with the shift/rotate count warnings is solely because the internal/workhorse/wrapped functions (e.g., _rotl16) specify *their* incoming shift/rotate count params as "unsigned char", while what is passed into them by the wrapper func is declared as an "unsigned int" - so of course this type of warning will be issued. :)

But the cast to "silence" these is only operating inside of these wrappers, and so has no direct interaction with callers of the wrappers - and don't believe the inlining changes this from a code-analysis standpoint.

None of this discussion is about the code sites *calling* these wrappers, whether from inside the library or outside.

> We can possibly do something like: find all library callers (use case
> (1)), and ensure they don't trigger the warning. Then, any warnings
> that remain are user's code (use case (2)).
>
> I think we really need a rotate that operates on a data type larger
> than a byte. We can possibly do that with inline assembly on x86. But
> we won't really have a solution for x64 unless we provide MASM64
> routines in a stand alone source file (x64 lacks inline assembly).
>
>> Further, I might be inclined to also roll back the commit that added all the
>> static_cast<word16> stuff to this block - at least if it was motivated by
>> thinking that was the cause of the C4242 warnings - since the type being
>> returned by the wrapped functions is already compatible with the the types
>> returned by the wrapper functions.
>
> I think that was a different complaint. I think we are getting them
> piecemeal because of the 'pragam once'. The idea was to avoid flooding
> the code with all the warnings.

I don't think I can say this much differently - I can only restate that I am surprised that the "static_cast<word16>" constructs added to the returned values of these wrappers were necessary for any compiler, given that the "unsigned short" return values from the wrapped functions are either the same as what "word16" is actually defined as (in MSVC-land), or at the very least, assignment-compatible with in some other compiler.

And ALL of my comments are about the range in misc.h that is conditionalized as

#if _MSC_VER >= 1400 && !defined(__INTEL_COMPILER)

> I'll verify it once I get a Windows VM fired up.
>
> Jeff

Cool, thanks. :)

Robert

[Jeffrey Walton]

On Tuesday, July 14, 2015 at 8:11:53 PM UTC-4, Robert Roessler wrote:

Jeffrey Walton wrote:
> On Tue, Jul 14, 2015 at 6:20 PM, Robert Roessler wrote:
>> Jeffrey Walton wrote:
>>>
>>> ...
>>> OK, the additional warnings on Visual Studio likely came because of
>>>
>>> https://github.com/weidai11/cryptopp/commit/e67480dd9e8dcda57939fc7480e273b8d2e81335.
>>>
>>> We also cleared some more misc.h warnings  at
>>>
>>> https://github.com/weidai11/cryptopp/commit/828c550389f79b035f4518a79b7138d281f0961c.
>>>
>>>> believe the actual complaint from VS (2013 for me) is caused by the
>>>> various
>>>> low-level workhorse "rotate" functions declaring their "shift" param as
>>>> an
>>>> unsigned char - which is where the actual alleged truncation is taking
>>>> place
>>>> (or would).

I have not forgot about this. I'm untangling the difference between rotrFixed, rotrVariable and rotrMod (and friends).

The next check-in will have definitions or expected behaviors for them. Once everyone knows what should be happening, then we can tackle policy decisions, like "do we assert for external library callers".

Jeff

[Jean-Pierre Münch]

Am 14.07.2015 um 03:02 schrieb Jeffrey Walton:

Hi Everyone,

I've avoided or side stepped this issue, but Jonathan Wakely brought it to light recently after looking at algparam and observing:

> AssignFromHelperClass binds a
> reference to NULL just to default-construct a temporary of some type, then
> binds a const-reference to that temporary, then casts away const to modify
> the value of that temporary. What seems to be a "VC60 workaround" introduces
> undefined behaviour by trying to be far too clever.

Jonathan's observations highlight some of the issues with supporting older platforms and gear. The older gear includes compilers like Microsoft's embedded eVC 4.0 compiler, and older IDEs like a 1990's era Code Warrior. I think the support was cutting edge back then, but its a hindrance or a detriment today.

My QUESTIONS are:

 (1) Where can we reasonably draw a line?

I think we may want to handle this as Microsoft used to do:
10 years support.
We publish a final version which still supports the compiler (-> next one should still support at least VS 2005), tell people we'll remove that support with next patch and that's it.

 (2) How should we proceed moving forward?

10 years support.
If we have a workaround for anything 10 years or more older we'll deprecate it and drop it with next release (f.ex. keep everything for 5.7.0 and drop with 5.8.0 / 5.7.1 so everyone has a few months / years to prepare)

 (3) And how can we test that we are proceeding as agreed?

Maybe look for the relevant macros? (GNUC_MAJOR, GNUC_MINOR, MSC_VER, ...)

My personal feelings lie somewhere around:

 * Circa 2000's compilers and IDEs

<2005, drop it.

 * C++03 support

2005 and newer compiler still rely on this -> can't drop

 * GCC 4.2.1 due to OpenBSD

July 2007, still within 10 years time-frame

Note that the above does not say "No, we don't support C++11". We just have to maintain sources that are compatible with the project's goals and objectives. I've already started exploring this issue at "How to guard move constructors for C++03 and C++11?", http://stackoverflow.com/q/30797388.

Once we classify an item (like, say GCC 2.9 or 3.x; or Visual Studio 5.0 project files), what do we do with them. So my QUESTION here is:

  (4) What do we do with obsolete gear once we agree something is obsolete?

deprecate it once it hits our EOL (10 years) -> compiler warning?
After this either

1. Remove it
2. #if 0 it

I'd prefer (1) as it's more like OpenBSD did with LibreSSL and we don't actually need this code if we don't advertise the support but there may be good reasons for (2).

I hope I understood everything correctly :)

BR

JPM

Jeff

--
--
You received this message because you are subscribed to the "Crypto++ Users" Google Group.
To unsubscribe, send an email to cryptopp-user...@googlegroups.com.
More information about Crypto++ and this group is available at http://www.cryptopp.com.
---
You received this message because you are subscribed to the Google Groups "Crypto++ Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cryptopp-user...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.