Preparations to make RIOT 64-bit ready #20154

OlegHahm · 2023-12-05T22:35:40Z

Adopted from #19890

Contribution description

Following @benpicco's proposal I handpicked the patches from #19890 that only prepare the code base for 64bit support.

All changes are done by @fzi-haxel.

Testing procedure

All tests and applications should remain unchanged for existing platforms.

benpicco

This all looks reassuringly straightforward!

pkg/flashdb/patches/0002-64bit-fix.patch

benpicco · 2023-12-06T17:51:08Z

pkg/micro-ecc/patches/0001-No-128-Bit-types-on-gcc.patch

+ #endif
+
+-#if defined(__SIZEOF_INT128__) || ((__clang_major__ * 100 + __clang_minor__) >= 302)
+#if ((__clang_major__ * 100 + __clang_minor__) >= 302)


that one I don't understand

good question - @fzi-haxel?

On x86_64, __SIZEOF_INT128__ is defined, therefore __int128 is used, but this leads to this GCC warning:

build/pkg/micro-ecc/types.h:98:18: error: ISO C does not support ‘__int128’ types [-Werror=pedantic] 98 | typedef unsigned __int128 uECC_dword_t;

In retrospect, simply not using __int128 does not seem to be the most elegant solution. Another option would be:

__extension__ typedef unsigned __int128 uECC_dword_t;

However, the GCC documentation points out the following when using __extension__

only system header files should use these escape routes.

What do you think?

I'm a bit confused: clang accepts __int128 even though it is not part of the C standard but gcc does not?

What I also do not understand: if __int128 is a built-in data type on gcc (>= 4.1) wouldn't I still get the warning even with the typedef? (Which definitiv would be actually used?)

If -Wpedantic is enabled, yes.
Example: https://godbolt.org/z/bsYq49jGY

Using __extension__ would get rid of the warning.

Hm, actually, I tried this (using gcc 13.2.1) but still got the warning.

Are you sure? I just tested it with (13.2.0) and didn't get this warning.

But GCC 13 now gives this warning during linking

/usr/bin/ld: warning: /work/tests/pkg/micro-ecc/bin/native/cpu/tramp.o: missing .note.GNU-stack section implies executable stack

Ah, no, sorry, I think there was a PEBCAK issue (I always confuse the order of arguments for typedef).

Opened an issue upstream with some suggested solutions

benpicco · 2023-12-06T17:52:23Z

pkg/micropython/patches/0001-Ensure-mp-int-and-uint-have-pointer-size.patch

+-typedef int mp_int_t;       // must be pointer size
+-typedef unsigned mp_uint_t; // must be pointer size
+typedef intptr_t mp_int_t;       // must be pointer size
+typedef uintptr_t mp_uint_t; // must be pointer size


Since we are using our own fork anyway, you might as well open the PR there.

~~True - but actually unrelated to this PR, isn't it?~~

Sorry - got confused by the highlighting for the patch file here. I thought that was an actual diff instead of an added file.

Made a PR in the fork.

PR is merged, waiting for #20208 to get merged before we can drop this patch.

pkg/tinydtls/patches/0002-fix-printf-warning-for-64-bit-systems.patch

riot-ci · 2023-12-06T19:08:22Z

Murdock results

❌ FAILED

3d02eb6 Revert "examples/dtls-sock: Removed unnecessary FEATURES_REQUIRED"

Success	Failures	Total	Runtime
5638	0	8098	07m:16s

Artifacts

OlegHahm · 2023-12-07T14:34:56Z

Btw @fzi-haxel by no means I wanted to take over your contribution. I just got excited about your PR and the idea to have (finally) 64 bit support (for native) in RIOT and I thought that @benpicco had a good point that we could potentially speed up things by splitting the PR. Since the PR did seem stall somewhat, I just moved forward without asking you first. I hope that was okay with you!?

Fixed compilation errors. Mostly DEBUG/printf formatting and void pointer casting. Added another hardcoded offset for gnrc_sixlowpan_frag_* tests. Increased mem size of lua test in case of 64 bit. Reduced required backtrace size to 3 in native backtrace test.

Created separate PR (RIOT-OS#20165) This reverts commit 055471a.

Merged in upstream

Fix for failed murdock test

Additional bug found by compiling all tests with llvm. Interestingly, this didn't crash the 64 bit tests before nor did it give a warning in GCC.

Added missing DOXYGEN documentation

OlegHahm · 2023-12-21T16:32:34Z

I rebased on current master (including @fzi-haxel's size_t formatters) and fixed the resulting merge conflicts.

fzi-haxel · 2023-12-21T19:58:49Z

I pushed three more PRs to Olegs branch:

examples/dtls-sock: Revert changes due to separate PR
Created examples/dtls-sock: Enable non 32-bit architectures #20196 as this is technically a general issue. It would be very helpful if someone with a compatible board could test it.
(This should hopefully fix the murdock test)
drivers: Missing size_t format specifiers
I seem to have missed some size_t format specifiers in drivers that still use an unsigned cast in this PR.
pkg: drop micropython patches
Dropped micropython patches due to pkg: micropython: bump version #20208

Merge as you deem fit

Dropped micropython patches due to upstream merge

Created separate PR (RIOT-OS#20196) This reverts commit 09f4453.

fzi-haxel · 2023-12-22T09:53:34Z

This should hopefully fix the murdock test

Or not...
I also re-ran native64 tests after the rebase and found even more problems.

I added a new PR and compiled all examples/tests for avr-rss2, iotlab-m3, sipeed-longan-nano-tft, esp32-wroom-32, native and "native64".

fzi-haxel · 2024-01-15T12:00:55Z

I think this PR has become a bit complicated to review and is probably still a bit too big to be easily discussed. I believe the best way to proceed with the PR is to break it down into smaller PRs that can be merged step by step.
I updated #19890 to reflect the state of this discussion and marked it as WiP.

OlegHahm · 2024-01-29T09:57:57Z

Probably you're right. Should I fix this one?

fzi-haxel · 2024-01-30T08:55:32Z

Probably you're right. Should I fix this one?

All commits and review change requests in this PR should now be included in other smaller PRs.
The main PRs are

Therefore, I think you could close this PR.
Many thanks to the reviewers and especially to @OlegHahm for the effort.

OlegHahm added Platform: native Platform: This PR/issue effects the native platform Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Dec 5, 2023

OlegHahm requested review from leandrolanzieri, jia200x, MrKevinWeiss, miri64, smlng, aabadie, MichelRottleuthner, fjmolinas, kaspar030, bergzand and haukepetersen as code owners December 5, 2023 22:35

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 6, 2023

benpicco reviewed Dec 6, 2023

View reviewed changes

OlegHahm mentioned this pull request Dec 6, 2023

Fixing some data types kaspar030/micropython#4

Merged

fzi-haxel and others added 12 commits December 21, 2023 17:30

fixup! tests: 64 bit compatibility

a9dd66b

fixup! tests: 64 bit compatibility

9a4549b

fixup! core: 64 bit compatibility

2157de4

fixup! drivers: 64 bit compatibility

c175f02

unittests/hashs: Fixed wrong string length

e3f34ef

Revert "unittests/hashs: Fixed wrong string length"

c4181d6

Created separate PR (RIOT-OS#20165) This reverts commit 055471a.

fixup! tests: 64 bit compatibility

6081fb2

fixup! pkgs: 64 bit compatibility

71e49c5

Merged in upstream

fixup! tests: 64 bit compatibility

464f511

Fix for failed murdock test

fixup! pkgs: 64 bit compatibility

a6e7cca

Additional bug found by compiling all tests with llvm. Interestingly, this didn't crash the 64 bit tests before nor did it give a warning in GCC.

fixup! sys: 64 bit compatibility

47ce2a2

Added missing DOXYGEN documentation

OlegHahm force-pushed the pr/64bit-preparation branch from a15c541 to 47ce2a2 Compare December 21, 2023 16:31

github-actions bot removed the Area: OTA Area: Over-the-air updates label Dec 21, 2023

fzi-haxel added 3 commits December 21, 2023 21:37

fixup! pkgs: 64 bit compatibility

60a2241

Dropped micropython patches due to upstream merge

fixup! drivers: 64 bit compatibility

e0889fb

Revert "examples/dtls-sock: Removed unnecessary FEATURES_REQUIRED"

3d02eb6

Created separate PR (RIOT-OS#20196) This reverts commit 09f4453.

This was referenced Jan 12, 2024

pkg/ccn-lite: Bump version #20248

Merged

64 bit: Initial support for Linux x86-64 native. #19890

Closed

This was referenced Jan 15, 2024

core, sys, drivers: 64-bit support preparations #20257

Merged

pkg: 64-bit support preparations #20271

Merged

tests: 64-bit support preparations #20276

Merged

fzi-haxel mentioned this pull request Jan 30, 2024

native: Linux/x86_64 support #20315

Merged

OlegHahm closed this Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparations to make RIOT 64-bit ready #20154

Preparations to make RIOT 64-bit ready #20154

OlegHahm commented Dec 5, 2023

benpicco left a comment

benpicco Dec 6, 2023

OlegHahm Dec 6, 2023

fzi-haxel Dec 7, 2023

OlegHahm Dec 7, 2023

fzi-haxel Dec 7, 2023

fzi-haxel Dec 7, 2023

OlegHahm Dec 7, 2023

fzi-haxel Dec 7, 2023

OlegHahm Dec 7, 2023

fzi-haxel Dec 8, 2023

benpicco Dec 6, 2023

OlegHahm Dec 6, 2023 •

edited

Loading

OlegHahm Dec 7, 2023

OlegHahm Dec 21, 2023

riot-ci commented Dec 6, 2023 •

edited

Loading

OlegHahm commented Dec 7, 2023

OlegHahm commented Dec 21, 2023

fzi-haxel commented Dec 21, 2023 •

edited

Loading

fzi-haxel commented Dec 22, 2023

fzi-haxel commented Jan 15, 2024

OlegHahm commented Jan 29, 2024

fzi-haxel commented Jan 30, 2024

Preparations to make RIOT 64-bit ready #20154

Preparations to make RIOT 64-bit ready #20154

Conversation

OlegHahm commented Dec 5, 2023

Contribution description

Testing procedure

benpicco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegHahm Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Dec 6, 2023 • edited Loading

Murdock results

Artifacts

OlegHahm commented Dec 7, 2023

OlegHahm commented Dec 21, 2023

fzi-haxel commented Dec 21, 2023 • edited Loading

fzi-haxel commented Dec 22, 2023

fzi-haxel commented Jan 15, 2024

OlegHahm commented Jan 29, 2024

fzi-haxel commented Jan 30, 2024

OlegHahm Dec 6, 2023 •

edited

Loading

riot-ci commented Dec 6, 2023 •

edited

Loading

fzi-haxel commented Dec 21, 2023 •

edited

Loading