Swap endian-ness in tools on big endian hosts #329

zeldin · 2021-04-10T11:25:38Z

When the host running the SDK is not little endian, all data that is exchanged with the Pico must be converted to/from little endian byte order.

Instead of sprinking explicit swaps around the code, I used available mechanisms for doing the swaps implicitly. Unfortunately, there is no such mechanism that works with both C and C++, so because the code base mixes C and C++ double mechanisms need to be employed...

The C++ fix also suffers from the fact that some structures are declared as __attribute__((packed)), which requires the contents to be POD. Because of this, the little_endian template is not allowed to have a constructor. This is not a problem in the SDK itself, but in picotool it would have been convenient to be able to create std::vector<little_endian<uint32_t>> program with an initializer.

lurch · 2021-04-10T11:59:23Z

@kilograham may feel differently (and his opinion matters much more than mine), but I think it might be simpler to keep everything on the Pico little-endian, and rely on the host-side tools (elf2uf2, picotool, etc.) to do any necessary conversion if the host itself isn't little-endian?
Otherwise you might have some Picos presenting a little-endian host interface, and other Picos presenting a big-endian host interface, depending only on what firmware has been flashed to them, and that might get a bit confusing?

(Of course, that's assuming that I'm correctly understanding what this PR is trying to do, which I might not be? 🤷 )

zeldin · 2021-04-10T12:05:00Z

@lurch I think there may be a misunderstanding here. What the code does it make the data being accessed as little endian, regardless of the systems actual endianness. On a system which is already little endian, such as the Pico itself, this is a no-operation (and in fact the macros will expand to nothing). There is nothing here that will make anything big endian on the Pico. All conversions happen on the host side (and only if the host is big endian).

lurch · 2021-04-10T12:16:20Z

All conversions happen on the host side (and only if the host is big endian).

Cool, please accept my apologies for my misunderstanding (I'm more of a Python guy than a low-level C guy). Is a similar patch needed for https://github.com/raspberrypi/picotool or does this PR already encompass that?

zeldin · 2021-04-10T12:18:45Z

@lurch Yes, I have a patch for picotool too. It depends on this patch though, so I wanted to test the waters here first before submitting it. 😄

lurch · 2021-04-10T12:21:44Z

Just out of curiosity, what big-endian host machine / OS / environment are you using?

zeldin · 2021-04-10T12:25:02Z

@lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. 😸

aallan · 2021-04-10T12:53:50Z

@lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. 😸

I'd presumed you were on a Dec Alpha or a VAX or some such. Huh. I hadn't quite grokked that the Pi was Bi-endian.

zeldin · 2021-04-10T13:08:11Z

@aallan All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. 😆

aallan · 2021-04-10T13:15:09Z

All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. 😆

Having used a VAX a lot during my undergrad I have total false memories of it being big endian at that point. Huh. Guess I was brain washed by my many years on Dec Alphas during my postgrad years. 🤷‍♂️

kilograham · 2021-04-15T20:36:27Z

can you rebase this on develop

zeldin · 2021-04-16T16:58:27Z

@kilograham Sure thing!

kilograham · 2021-04-18T23:00:28Z

I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode). I don't believe we have any non target device which touches this which isn't C++ anyway.

I'd be tempted perhaps to do (i will keep trying to think of a nice way to do this with typedefs - I guess we can guard with an #ifdef)

#ifndef le_uint32_t
#define le_uint32_t uint32_t
#endif

in the headers (or in a common one if we have one which works across elf2uf2, picotool, regular build etc)

And that way we defer everything to the user.

C code, can continue to have to reverse byte orders manually (or wrap the structure with another one which reverses the byte order using scalar_storage_order - if that's what they choose to do)

In our C++ we can define le_uint32_t to be stored_little_endian<uint32_t> before including the header

zeldin · 2021-04-19T20:41:22Z

I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode).

Ah, ok. In that case at the very least there should be a conditional #error to catch the case where you use clang on a big-endian host. But not using it at all might be preferable.

I don't believe we have any non target device which touches this which isn't C++ anyway.

Maybe I mistunderstand your point here, but C code compiled for the host using the relevant structs exist in the file picoboot_connection.c (which is in picotool, but the header file picoboot.h that it gets the structs from is in pico-sdk).
Of course, changing this file to C++ could be an option.

In our C++ we can define le_uint32_t to be stored_little_endian<uint32_t> before including the header

Right, but wouldn't it make sense for platform.h to make that define and provide the appropriate template? If you don't like e.g. uf2.h getting an added #include "pico/platform.h", the C++ source code could be required to include it from the top level instead. Or I guess a new header file with a different name. It just doesn't seem like this wheel should have to be reinvented by each client. 😸

zeldin · 2021-05-01T13:12:33Z

@kilograham I reworked the patch to work along the lines you described. The SDK patch wasn't affected that much. The picotool patch became a bit more messy when picoboot_connection.c can't rely on __attribute__((scalar_storage_order)) anymore, but it's not too bad. I'll open a PR for that repo to so you can take a look.

kilograham

a few thoughts; will look at the picotool patch too

src/common/pico_binary_info/include/pico/binary_info/structure.h

kilograham · 2021-05-01T19:47:46Z

src/host/pico_platform/include/pico/platform.h

@@ -135,5 +135,21 @@ static inline void __compiler_memory_barrier(void) {
 }
 #ifdef __cplusplus
 }
+


note we should only need this for big endian systems... either omit, or make it a no-op on little endian host

Sure. A modern compiler will in fact already turn this code into a no-op on little endian, but I can put an explicitly no-op version inside an #ifdef.

(Here's what I mean about the compiler fixing this for you, btw: https://godbolt.org/z/3cTdTYGzY)

Yeah, I figured, I didn't know they all did so so uniformly... still it is clearer, and some maniacs may use -O0.

kilograham · 2021-05-01T19:48:02Z

tools/elf2uf2/main.cpp

 #include <algorithm>
+#include "pico/platform.h"
+#define le_uint16_t stored_little_endian<uint16_t>


this could be in platform.h

Ok, I guess if you want an alternate implementation you could make your #defines before including pico/platform.h instead of after.

…e endian

kilograham · 2021-06-01T16:12:46Z

Moved to 1.2.1 as I think i want to put the stuff in platform.h after all

josch · 2024-06-05T09:51:05Z

We are now carrying this as a patch to the Debian package of pico-sdk so that it works on s390x:

https://salsa.debian.org/debian/pico-sdk/-/blob/master/debian/patches/329.patch?ref_type=heads

This closes raspberrypi/picotool#104

josch · 2024-06-06T06:29:22Z

FYI, the pico-sdk autopkgtest failed on (big endian) s390x before but with the (rebased) patch from this pull request it passes now: https://ci.debian.net/packages/p/pico-sdk/testing/s390x/47384722/

kilograham changed the title ~~Properly account for Pico being little endian~~ Swap endian-ness in tools on big endian hosts Apr 11, 2021

zeldin changed the base branch from master to develop April 16, 2021 16:58

zeldin force-pushed the endianness branch from 6d567af to 36a6ee4 Compare April 16, 2021 16:58

Swap endian-ness in tools on big endian host

b1a6158

zeldin force-pushed the endianness branch from 36a6ee4 to b1a6158 Compare May 1, 2021 13:09

zeldin mentioned this pull request May 1, 2021

Swap endian-ness in tools on big endian host raspberrypi/picotool#28

Open

kilograham reviewed May 1, 2021

View reviewed changes

zeldin added 2 commits May 2, 2021 12:40

Move default C++ definitions of le_ types to platform.h

d9e77e0

Provide a specialized implementation of stored_little_endian on littl…

8babb2c

…e endian

kilograham added this to the 1.2.1 milestone May 31, 2021

kilograham modified the milestones: 1.2.1, 1.3.0 Oct 6, 2021

kilograham assigned will-v-pi May 19, 2024

kilograham added pioasm elf2uf2 labels May 19, 2024

kilograham added the review label May 19, 2024

lurch mentioned this pull request Jun 5, 2024

elf2uf2 on s390x: ERROR: Not an ELF file raspberrypi/picotool#104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap endian-ness in tools on big endian hosts #329

Swap endian-ness in tools on big endian hosts #329

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

aallan commented Apr 10, 2021 •

edited

Loading

zeldin commented Apr 10, 2021

aallan commented Apr 10, 2021

kilograham commented Apr 15, 2021

zeldin commented Apr 16, 2021

kilograham commented Apr 18, 2021

zeldin commented Apr 19, 2021

zeldin commented May 1, 2021

kilograham left a comment

kilograham May 1, 2021

zeldin May 1, 2021

zeldin May 2, 2021

kilograham May 2, 2021

kilograham May 1, 2021

zeldin May 1, 2021

kilograham commented Jun 1, 2021

josch commented Jun 5, 2024

josch commented Jun 6, 2024

Swap endian-ness in tools on big endian hosts #329

Are you sure you want to change the base?

Swap endian-ness in tools on big endian hosts #329

Conversation

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

lurch commented Apr 10, 2021

zeldin commented Apr 10, 2021

aallan commented Apr 10, 2021 • edited Loading

zeldin commented Apr 10, 2021

aallan commented Apr 10, 2021

kilograham commented Apr 15, 2021

zeldin commented Apr 16, 2021

kilograham commented Apr 18, 2021

zeldin commented Apr 19, 2021

zeldin commented May 1, 2021

kilograham left a comment

Choose a reason for hiding this comment

kilograham May 1, 2021

Choose a reason for hiding this comment

zeldin May 1, 2021

Choose a reason for hiding this comment

zeldin May 2, 2021

Choose a reason for hiding this comment

kilograham May 2, 2021

Choose a reason for hiding this comment

kilograham May 1, 2021

Choose a reason for hiding this comment

zeldin May 1, 2021

Choose a reason for hiding this comment

kilograham commented Jun 1, 2021

josch commented Jun 5, 2024

josch commented Jun 6, 2024

aallan commented Apr 10, 2021 •

edited

Loading