[CLN-list] CLN and GiNaC patches for Win64 build

[Jan Rheinländer]

Hello Bruno,

thanks for your detailed comments. I do not have the "big picture" of
all the supported platforms so I put in a lot of conditional code to be
on the safe side. I will re-work the patches as you suggest.

About the "static initialization order fiasco": I learned about this by
searching the web. It seems that the C++ standard does not guarantee any
kind of order for the initialization of static variables. Therefore, it
is unknown when exactly free_hook and malloc_hook pointers will be
assigned a value. I verified it in the Visual Studio debugger.
integral::relative_integration_error is is an expression that is
initialized as 1e-8 which results in constructing a cl_DF. This requires
malloc_hook and free_hook to be initialized, but in my case only
malloc_hook was, whereas free_hook was 0x0.

free_hook is invoked here (the temporary cl_DF is deleted when the
method returns).

inline const cl_F cl_float (double x, float_format_t y)
        { return cl_float(cl_DF(x),y); }

I don't see any way around this problem other than removing the
user-definable hooks (certainly nobody is using those hooks on Windows
x64 platform currently ...)

About the "Force correct underlying type of float_format_t for Win64
platform": I posted about this previously (12.3.2018). The problem
certainly exists with VS 2015 compiler.

Jan


Am 01.05.2018 um 19:15 schrieb Bruno Haible:
> Jan Rheinländer wrote:
>> I reviewed Robert's patches and added some of my own, that were
>> necessary for GiNaC to work with CLN on Win64. They can be viewed on Github:
>>
>> === CLN ===
>>
>> https://github.com/jrheinlaender/cln/commits/win64
>>
>> There are two kinds of patches:
>>
>> - Patches required for Win64 platform (e.g. UL is 32 bit type on that
>> platform). Mostly due to Robert (Number 1-10)
>>
>> - Cosmetic patches to avoid unnecessary compiler warnings (e.g.
>> confusion of class and struct)
>>
>> Most patches I surrounded with #ifdef _M_AMD64 so that other platforms
>> will be unaffected
> Patch 01/10 looks good to me. All relevant platforms have <stdint.h> nowadays.
> The last one to add it was MSVC, in the MSVC 2015 release. The last platform
> that does NOT have <stdint.h> is IRIX 6.5, which is not a porting target any
> more.
>
> Patch 02/10 looks only partially right.
> The #include <stdint.h> and the changes to sintP, uintP, sintV, uintV should
> become unconditional.
> On the other hand, the changes to sint32, uint32, sint64, uint64 look either
> broken or - in the best case - worthless and confusing.
>
> Patch 03/10 is only partially right:
> * Include <stdint.h> always. Including it on a particular platform only will
>   lead to maintenance problems.
> * It is wrong to test _WIN64 like this. The macro _WIN64 is defined
>     - in 64-bit mingw,
>     - in 64-bit MSVC,
>     - in 64-bit Cygwin *if and only if* some Windows API header gets included.
>       See the file /usr/include/w32api/_cygwin.h on a 64-bit Cygwin:
>
>       /* _WIN64 is defined by the compiler specs when targeting Windows.
>          The Cygwin-targeting gcc does not define it by default, same as
>          with _WIN32.  Therefore we set it here.  The result is that _WIN64
>          is only defined if Windows headers are included. */
>       #ifdef __x86_64__
>       #define _WIN64
>       #endif
>
>   The correct test for the 64-bit native Windows platform is therefore
>     defined(_WIN64) && !defined(__CYGWIN__)
>
> * In the definitions of cl_tag_mask and cl_value_mask, it is simpler to write
>     (uintptr_t)1
>   instead of
>     (uintptr_t)1UL
>
> * The cl_combine overloads should be adjusted to be consistent with patch 02/10.
>
> Patch 04/10 looks good.
>
> Patch 05/10 is essentially good.
> Here too, include <stdint.h> always.
>
> Patch 06/10:
> Please add an include <stdint.h> here too. Otherwise there is a risk that we
> use the undefined type 'intptr_t'.
>
> Patch 07/10:
> * In cl_macros.h:
>   Please simplify this: Instead of adding a case for '#if defined(_M_AMD64)',
>   just change the !HAVE_FAST_LONGLONG case, replacing
>   1L by (intptr_t)1
>   2L by (intptr_t)2
>   -1L by -(intptr_t)1
>   -2L by -(intptr_t)2
> * In cl_DS_mul_fftp.h there is no need to use intptr_t. The 'long' type was
>   used here only to make sure we work on 32-bit words, as opposed to 16-bit
>   words (in the Windows 3.1 port). For 15 years, we can assume 32-bit 'int'
>   types already. Therefore just omit the 'L' suffix here.
>
> Patch 08/10:
> This looks wrong. I don't think you should cast an index of type 'size_t', ever.
> Can you explain the problem/issue?
>
> Patch 09/10:
> This patch should be dropped, once you have stopped fiddling with
> sint32, uint32, sint64, uint64 in patch 02/10.
>
> Patch 10/10:
> Include <stdint.h> always, here too.
> Other that that, this patch looks OK.
>
> Patch "Add missing delete operators to avoid compiler warning"
> * The 'throw' declarations are correct.
> * void operator delete (void* ptr, void* _ptr) { (void)ptr; (void)_ptr; }
>   1) This exists only since C++14. So it is a portability problem to use it.
>   2) Isn't it a memory leak if you define these methods to be a no-op?
>   Reference: http://en.cppreference.com/w/cpp/memory/new/operator_delete
>
> Patch "Remove user-definable malloc_hook and free_hook"
> No no, you can't just remove a documented API like that.
> What is the problem ("static initialization fiasco")? It must have a better
> solution.
>
> Patch "Fix bugs that result from the fact that type UL is 16bit on Win64"
> * Please don't write "Win64" platforms. That sounds like it would be a "win"
>   to use such a platform. Better write "64-bit Windows" instead.
> * 'unsigned long' is 32-bit, not 16-bit, no?
> * The cl_low.h change looks correct.
> * In cl_I_doublefactorial.cc and cl_I_factorial.cc please don't duplicate code.
>   How about writing (uintptr_t)xxx instead of xxxUL. Will that fix the issue?
>
> Patch "Force correct underlying type of float_format_t for Win64 platform"
> * What is the issue/symptom, and why does it not appear with other compilers
>   than MSVC?
>
> Patch "Fix an inconsistency (class used in definition where struct ..."
> Looks good. In the old times, CLN required g++, and for g++ 'friend class'
> and 'friend struct' are/were synonymous.
>
> Bruno
>
>