Hi, thanks for this. I should have asked a more specific question. What do you think I need to do to port CLN to LLP64 model. Even more specifically where do I need to change 'long' to intptr_t or the (unsigned version) if that is the right solution. I was thinking to replace 'long' everywhere, but that seems rather brutal. Also if I manage to carry this out, would you be interested in merging it?

It's a type tag. Its purpose is to distinguish between the immediate values (small integers, short floats) and the actual pointers. See include/cln/object.h for more details.

Quite interesting way of saving space. Thanks, Robert

Hi Alexei, in the end I managed to make a patch that works and a rather clean one in my view. What I did was the following - changed long to intptr_t and unsigned long to uintptr_t in all the hash code related files. - change the char* bit size comparison with long to intptr_t in both autoconf/intparams.c and m4/intparams.m4 - in types.h redefined 32 and 64 types in terms of (u)intptr_t if they match this is necessary when overloading functions so that there will be no duplicate overloads - In object.h cl_I is now defined cl_int and cl_uint are now defined by intptr_t and uintptr_t therefore overloaded functions are ifdefed out when the argument bitsize is the same as the pointer size to avoid clash. - changed the printing routines in io.h and their implementations to accept intptr_t and uintptr_t - in cl_macros.h the long and long long constants are casted into (u)intptr_t - since operator[] is overloaded many times except with std::size, so I needed to cast some vector indices to unsigned long from size_t. This was the largest index possible anyway. On win64 it will limit to 32 bits unfortunately. Someone might want to rethink how operator[] is overloaded. - there are three WIN64 ifdefs 1. word alignment is only 4 2. integer/conv/cl_I_from_UL.cc and integer/conv/cl_I_from_L.cc has to be compiled for WIN64 otherwise this conversion is missing during linking. The patch is tested with 'make check' on 3 OSes times 2 bit sizes 32/64 bit OS X Lion 32/64 Linux on Fedora 17 32/64 Windows through wine with the MINGW32/64 cross compilers the patch is unfortunately monolithic, I cannot break it up otherwise it does not compile. It is after all only changing long into intptr_t and dealing with the dependencies. It should not affect performance, it should leave things invariant for anything other than WIN64, since the assumption was that long == intptr_t. With best wishes, Robert On 13 Aug 2012, at 08:29, Alexei Sheplyakov wrote:

Hello,

On Fri, Aug 10, 2012 at 03:43:46PM +0100, Robert Szalai wrote:

...

What do you think I need to do to port CLN to LLP64 model.

long within CLN has (at least) 3 meanings:

1. An integer as large as a pointer. 2. On a 64-bit platform: a 64-bit integer. 3. Just long (say, in conversion functions).

One need to classify every occurrence and replace 1) with (u)intptr_t, 2) with int64_t, and leave 3) as is.

...

Also if I manage to carry this out, would you be interested in merging it?

That depends. A mega-patch without any explanation at all (like the one you've already sent) would be certainly rejected (sorry, we want CLN to be a bit maintainable).

The suggestions below can greatly increase the chances of your changes being accepted:

1. Split your changes into separate self-contained patches. For instance, if the changes include both bug fix and performance enhancement(s), separate those changes into two (or even more) patches. On the other hand, if you make a single change to numerous files (like API update), group those changes into a single patch. 2. Describe your changes: explain what problem you are trying to solve, what are you doing to solve it, and why you are taking a particular approach. 3. Mind the performance, please. 4. Don't introduce regressions (in particular, build failures are not appreciated at all). 5. Don't add (even more) compiler- and OS-specific code unless it's absolutely necessary. 6. Don't mix platform specific platform specific code with generic one (put the platform specific code into a different function, macros, file, etc).

@ developers: should we add `How to submit patch' entry to the FAQ?

...

...

It's a type tag. Its purpose is to distinguish between the immediate values (small integers, short floats) and the actual pointers. See include/cln/object.h for more details.

Quite interesting way of saving space.

The point is that small objects (integers) are allocated on the stack which is *much* faster than allocating them in the heap. So actually it's a way to improve the performance.

Best regards, Alexei

_______________________________________________ CLN-list mailing list CLN-list@ginac.de https://www.cebix.net/mailman/listinfo/cln-list

Hi Alexei, I've splitted the path into 10 parts. There is a bigger one that is only s/long/intptr_t/ and s/unsigned long/uintptr_t/ the rest is very small. Hopefully this can now be applied.

Ditto (the corresponding hunk is not quite optimal, though. {s,u}int32 can be typedef'ed to {,u}int32_t => no need for ugly #if's).

I was thinking of this, however we need exactly the same underlying type as (u)intptr_t for the same sized integer to avoid compiler confusion. So in some cases (e.g. on i686 Linux) stdint.h might define int32_t as int and intptr_t as long and in this case the compiler will complain about multiple definition of the same function. Bests, Robert

The ABI should stay the same for everything that was already working before.

Also, how am I supposed to apply the the patches? (I'm getting tons of failed hunks.)

Unzip the patches.tar.gz file and use 'git am 00*.patch' anywhere in the git repository of CLN. This way comments, authorship and everything is transferred properly. If it still does not apply let me know. Bests, Robert

Hi! On 09/24/2012 05:30 AM, Alexei Sheplyakov wrote:

I think the patches are not ready for inclusion yet. I'll send a pull request (or push directly to CLN repository if someone will grant me a commit access) after merging the patch.

Please get back to me if committing does not work for you.

...

(I assume that it doesn't change the binary interface on platforms where CLN is already working.)

Unfortunately the patch does change the API/ABI.

Ugh! But does it, really? The symbols (as of nm) are all the same after applying Robert's patch. Bye! -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>

Hi, 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 === GiNaC === https://github.com/jrheinlaender/ginac/commits/win64 - Here I removed some old patches that are not required any more because of improvements in MSVC 2015 - And added a few constructors from size_t I hope these patches are OK and can be included in master. Am 24.02.2018 um 12:49 schrieb Robert Szalai:

Here is the previous win64 patch set...

---------- Forwarded message ---------- From: "Robert Szalai" <robicjedi@gmail.com <mailto:robicjedi@gmail.com>> Date: Aug 15, 2012 02:22 Subject: Re: [CLN-list] MinGW64 build To: "CLN discussion list" <cln-list@ginac.de <mailto:cln-list@ginac.de>> Cc:

Hi Alexei,

I've splitted the path into 10 parts. There is a bigger one that is only s/long/intptr_t/  and s/unsigned long/uintptr_t/ the rest is very small.

Hopefully this can now be applied.

> Ditto (the corresponding hunk is not quite optimal, though. {s,u}int32 > can be typedef'ed to {,u}int32_t => no need for ugly #if's).

I was thinking of this, however we need exactly the same underlying type as (u)intptr_t for the same sized integer to avoid compiler confusion. So in some cases (e.g. on i686 Linux) stdint.h might define int32_t as int and intptr_t as long and in this case the compiler will complain about multiple definition of the same function.

Bests, Robert

_______________________________________________ CLN-list mailing list CLN-list@ginac.de https://www.cebix.net/mailman/listinfo/cln-list

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:

...

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

Jan Rheinländer wrote: 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

Hello Bruno, I placed an update to the patches on this branch: https://github.com/jrheinlaender/cln/commits/w64 Here is what came up when I re-worked the patches as suggested by you:

Patch 02/10 looks only partially right. On the other hand, the changes to sint32, uint32, sint64, uint64 look either broken or - in the best case - worthless and confusing. Yes those changes were unnecessary, I removed them.

Patch 03/10 is only partially right: * The cl_combine overloads should be adjusted to be consistent with patch 02/10. #ifdef HAVE_LONGLONG inline cl_uint cl_combine (cl_uint tag, unsigned long long value) { return cl_combine(tag, (cl_uint)value); } inline cl_uint cl_combine (cl_uint tag, long long value) { return cl_combine(tag, (cl_uint)value); } #endif

leads to a duplicate definition, because in types.h (after applying patch 02/10) we have typedef uintptr_t  uintP; typedef uintP  cl_uint; For the same reason, when pointer_bitsize == 64 then we need extra overloads for the signed and unsigned long types.

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?

The problem is that struct cl_SV in SV.h does not define any operator[](size_t index), which is required by cl_SV_copy.cc. All operator[] that are defined in cl_SV use the standard operator[](unsigned long index). Maybe this is intentional, that is, the index should not be larger than unsigned long. Otherwise, I suppose that struct cl_SV should be re-written to use operator[](size_t index) as the standard operator.

Patch 09/10: This patch should be dropped, once you have stopped fiddling with sint32, uint32, sint64, uint64 in patch 02/10. Even then, I think this patch is required because otherwise the constructors

cln::cl_I::cl_I(long) cln::cl_I::cl_I(unsigned long) will not have the required cl_I_constructor_from_L(long) cln::cl_I_constructor_from_UL(unsigned long) methods (linker error). In number.h, these are used on the condition that (long_bitsize==32) but in cl_I_from_L.cc they are only included on the condition that (cl_value_len < 32). For Windows x64 platform, cl_value_len works out to 62: cl_word_alignment: 4 cl_tag_len: 2 cl_value_shift: 2 pointer_bitsize: 64 intPsize:64 cl_pointer_size: cl_value_len: 64 - 2 I changed the condition in cl_I_from_(U)L.cc to #if (cl_value_len <= 32) || (long_bitsize == 32)

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 Actually, the compiler tells me that since no matching delete operator was defined, there will be a memory leak if during initialization an exception is raised: warning C4291: Kein zugehöriger delete-Operator gefunden; Speicher wird nicht freigegeben, wenn die Initialisierung eine Ausnahme auslöst But this is just a cosmetic patch anyway, I dropped it.

Jan

Dear Bruno, thank you for looking into this. I have moved on from CLN and GiNaC on Windows, because I felt it was unmaintained and there was no interest working with me on upstreaming my patch. I hope this time things will work out differently. Bests, Robert On Sun, Oct 27, 2019 at 6:56 PM Bruno Haible <bruno@clisp.org> wrote:

Hi Robert,

On 2018-02-24 you resent your mail and patches from 2012-08-15:

...

Here is the previous win64 patch set... ---------- Forwarded message ---------- From: "Robert Szalai" <robicjedi@gmail.com> Date: Aug 15, 2012 02:22 Subject: Re: [CLN-list] MinGW64 build To: "CLN discussion list" <cln-list@ginac.de> Cc:

Hi Alexei,

...

I've splitted the path into 10 parts. There is a bigger one that is

only

...

s/long/intptr_t/ and s/unsigned long/uintptr_t/ the rest is very small.

Hopefully this can now be applied.

...

Ditto (the corresponding hunk is not quite optimal, though. {s,u}int32 can be typedef'ed to {,u}int32_t => no need for ugly #if's).

I was thinking of this, however we need exactly the same underlying type as (u)intptr_t for the same sized integer to avoid compiler confusion. So in some cases (e.g. on i686 Linux) stdint.h might define int32_t as int and intptr_t as long and in this case the compiler will complain about multiple definition of the same function.

Bests, Robert

I have applied the 3 parts of your series that were good, and fixed the remaining problems. Indeed, the problems with the overload conflicts (in cl_combine, fprintdecimal, fprinthexadecimal) that are caused by the fact that intptr_t may be 'int', may be 'long', may be 'long long', were hardest to resolve. I chose not to introduce platform-dependent #ifs (too hairy to maintain), nor an autoconf test (because the include files are supposed to be usable by different compilers, and different compilers may define intptr_t differently), but instead use an inline function redirection.

Bruno

Hi Richy,

If I'm not mistaken, all this should not have broken the ABI on platforms that were already supported, right?

It has broken the ABI, unfortunately. Looking at the changes in the include files, excluding inline functions: - fprintdecimal (std::ostream& stream, unsigned long x) fprintdecimal (std::ostream& stream, long x) no longer exist. - fprintdecimal_impl (std::ostream& stream, uintptr_t x) fprintdecimal_impl (std::ostream& stream, intptr_t x) were added and are used by the new include files. Therefore someone who compiles with the old include files and links with the new library will see link errors (missing fprintdecimal symbols), and someone who compiles with the new include files and links with the old library will see link errors (missing fprintdecimal_impl symbols). fprintdecimal is documented. Bruno