New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

32-bit compatibility (64-bit gid on 32-bit architecture) #1065

Merged
merged 12 commits into from Nov 26, 2018

Conversation

Projects
None yet
5 participants
@gtrensch
Copy link
Contributor

gtrensch commented Oct 30, 2018

This pull request resolves #1031.

This pull request fixes an issue with compiling and running NEST on a 32-bit platform. It has been tested on a 32-bit ARM Raspberry Pi.

The source code changes do the following:

  • Let CMake identify when compiling for a 32-bit architecture and then include the appropriate headers. Even on 32-bit platforms there is usually an uint64_t available in stdint.h.
  • Define the gidexplicitly as 64-bit. Both, unsigned long and unsigend long long are not appropriate for this. They are platform-dependent data types!
  • The 64-bit gid on 32-bit required a few more changes down to sli-types, i.e., overloading constructors and ensuring that this does not conflict with data types on 64-bit platforms, i.e., uint64_t might be equivalent to unsigned long preventing overloading.

I found that it is not needed for the moment to change the types of index and thread as they are compiled from a subset of the bits still fitting 32-bit. Making this uint64_t breaks sli-types and will require more changes. Since we decided to follow the C++11 standard, I also introduced constexpr for the constant-definitions to ensure their evaluation at compile-time.

gtrensch added some commits Oct 26, 2018

- CMake check for 32-bit platform
- include stdint.h on 32-bit platforms
- use uint64_t instead of unsigned long and size_t to ensure 64-bit data types also on 32-bit platforms
@apeyser
Copy link
Contributor

apeyser left a comment

Very nice!

Show resolved Hide resolved nestkernel/nest_types.h Outdated
Show resolved Hide resolved nestkernel/source.h Outdated
Show resolved Hide resolved nestkernel/target.h Outdated
Show resolved Hide resolved nestkernel/target.h
Show resolved Hide resolved sli/token.cc
@gtrensch

This comment has been minimized.

Copy link
Contributor

gtrensch commented Nov 9, 2018

@apeyser please review the refactored version of the code. For the refactored experimental boost cpptests I suggest @jakobj as a reviewer.

The 64-bit target-id layout can now be specified in a more generic way. Bit-masks and max-values are generated by const-expressions. I also removed some of the raw numbers to get rid of dangerous logical dependencies!

@apeyser
Copy link
Contributor

apeyser left a comment

Only one comment requires any response: whether cmake should just fail for 32-bit but non-ILP32 data model machines (the unlikely but technically possible case). Currently, it should compile properly rather than failing before fairly cryptically with a template error --- but maybe it should fail explicitly if someone where to try that (and maybe no one ever will).

Show resolved Hide resolved cmake/CheckIncludesSymbols.cmake Outdated
Show resolved Hide resolved nestkernel/target.h
Show resolved Hide resolved nestkernel/target.h Outdated
@gtrensch

This comment has been minimized.

Copy link
Contributor

gtrensch commented Nov 12, 2018

@apeyser thanks for your comments! I have included your suggestions in the code.

@sanjayankur31
Copy link
Contributor

sanjayankur31 left a comment

The code bits look mostly OK to me. Has this been tested by CI on the different architectures? I'm testing it on the different arches Fedora supports at the moment. I'll have more to say once those builds finish.

set( CMAKE_EXTRA_INCLUDE_FILES "stdint.h" )
check_type_size( "long" LONG_SIZE )
if( NOT LONG_SIZE EQUAL 4 )
message( FATAL_ERROR "Platform not supported: None-ILP32 data model for 32 bit architecture." )

This comment has been minimized.

@sanjayankur31

sanjayankur31 Nov 15, 2018

Contributor
Suggested change Beta
message( FATAL_ERROR "Platform not supported: None-ILP32 data model for 32 bit architecture." )
message( FATAL_ERROR "Platform not supported: Non-ILP32 data model for 32 bit architecture." )
@@ -53,6 +53,30 @@ if ( UINT16_T_SIZE GREATER 0 )
set( HAVE_UINT16_T ON )
endif ()

set( CMAKE_EXTRA_INCLUDE_FILES "sys/types.h" )

This comment has been minimized.

@sanjayankur31

sanjayankur31 Nov 15, 2018

Contributor

I'm wondering of this some comments should be added here just to make it easier for devs in the future to understand why this is here?

This comment has been minimized.

@gtrensch

gtrensch Nov 23, 2018

Contributor

This macro needs to be set up before check_type_size() and loses scope after execution of check_type_size(). This is CMake magic. I think it is clear what this part of the CMake script does. The rest is somehow CMake-technical and I tend not to begin documenting it.

@sanjayankur31

This comment has been minimized.

Copy link
Contributor

sanjayankur31 commented Nov 15, 2018

Still getting build failures on the 2 32 bit architectures supported in Fedora: i686 and armv7hl:

I'm also uploading the build logs. These are "scratch" builds and will be deleted in few hours.

armv7hl-build-log.txt
i686-build-log.txt

@jakobj

jakobj approved these changes Nov 16, 2018

Copy link
Contributor

jakobj left a comment

thanks a lot, new code looks good. test cases look solid 👍 i just have one small question

* Exception to be thrown if an internal error occures.
* @ingroup KernelExceptions
*/
class InternalError : public KernelException

This comment has been minimized.

@jakobj

jakobj Nov 16, 2018

Contributor

why do we need this new exception? wouldn't a KernelException by sufficient?

This comment has been minimized.

@gtrensch

gtrensch Nov 23, 2018

Contributor

Are not all exceptions derived from the base class KernelException? The enumeration that I introduced in targets.h as well as the InternalError exception should make the code more robust and easier to extend - just in case that there is more than a single status bit needed in future.

@sanjayankur31

This comment has been minimized.

Copy link
Contributor

sanjayankur31 commented Nov 16, 2018

I'm afraid all the travis tests machines seem to be amd64 machines, so the tests don't really check these code changes. The code doesn't build in i686 and armv7hl for me (as commented above). Can someone please look at the attached logs to see if I've missed something there?

@gtrensch

This comment has been minimized.

Copy link
Contributor

gtrensch commented Nov 18, 2018

@sanjayankur31 Ich checked the logs. The "constructor ambiguity" issue was one of the problems I also went into when testing on the 32-bit ARM Raspberry. It should be resolved. One of the warnings shows:

warning: unsigned conversion from 'long long int' to 'size_t' {aka 'unsigned int'} changes value from '4611686018427387903' to '4294967295' [-Woverflow]
   static const size_t disabled_marker_ = 4611686018427387904 - 1; // 2 ** 62 - 1

I removed that raw number from the code. Can you please check whether you ran your tests on the correct branch?

@sanjayankur31

This comment has been minimized.

Copy link
Contributor

sanjayankur31 commented Nov 19, 2018

@gtrensch ah, I think I may have run the build before those commits. I'll re-run with the latest one in your branch and report back.

@sanjayankur31

This comment has been minimized.

Copy link
Contributor

sanjayankur31 commented Nov 19, 2018

@heplesser

This comment has been minimized.

Copy link
Contributor

heplesser commented Nov 20, 2018

@gtrensch Could you resolve the conflicts that have occurred (I assume #1062 may be at fault), then this one should be ready to be merged.

@gtrensch

This comment has been minimized.

Copy link
Contributor

gtrensch commented Nov 23, 2018

The conflict is resolved.

@heplesser heplesser merged commit 6b26ff5 into nest:master Nov 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment