flipper-io / flipper

Flipper is a development platform that can be controlled from any programming language.
https://www.flipper.io/
Apache License 2.0
70 stars 15 forks source link

Use `size_t` for the sizes of things. #33

Closed TravisWhitaker closed 8 years ago

georgemorgan commented 8 years ago

This hardcore rekt the push/pull implementation. I'm going to have to fix that now. :(

TravisWhitaker commented 8 years ago

Can you provide any details?

georgemorgan commented 8 years ago

I have no idea how it broke.

Mikejmoffitt commented 8 years ago

What about it does not work? Can you get any useful diagnostic information with verbose printing statements?

georgemorgan commented 8 years ago

The difference from size_t meaning uint32_t on the host and uint16_t on the device screwed some alignment up. We have to change the length based on the system, which we should use a custom type for.

TravisWhitaker commented 8 years ago

In commit 1724ac9622c5da77426b035eaeedd7406aa8f9f3 I only changed the types in function prototypes, so if you're talking about alignment within a packet I don't see how those changes alone could cause an alignment problem; right now in master there are no size_ts in any of the fmr packet structs, so that can't be the cause of an alignment issue either.

It is impossible for truncation to 16 bits (or 32 bits) to change a binary number's divisibility by one or two or four (or eight, or sixteen, ...), so that alone can't cause an alignment problem either.

What leads you to believe this is an alignment issue?

Checking the headers, it looks like size_t is uint16_t on the ATmega, uint32_t on the 4s; what alternative would you propose for a type we define ourselves?

TravisWhitaker commented 8 years ago

May we assume git bisect or similar was used to verify this change is the culprit?

Mikejmoffitt commented 8 years ago

From looking at the commit, it looks like Travis is right - size_t is only being used to disambiguate an argument that refers to intended length.

georgemorgan commented 8 years ago

Fixed it.

In short, the AVR’s calling convention handles int32 weirdly. I’ve written some assembly code that knows how to load them, and as a result, FMR has to send int32’s in a specific way. They’re staged into the packet as an fmr32(…) if you want it loaded as an int32. When we changed to size_t, the firmware for the AVR was recompiled to only read a int16_t - making the alignment for the load (from the assembly code’s perspective) 2 registers off (8 bits per register in the calling convention). So, AVR normally loads LH LH for 32bit. This time, it was loading LH —, getting bogus data from the packet somewhere unrelated to the length, causing it to load an indeterminate number of bytes, overflow, and crash…

So the FMR sent a int32, the AVR read an int16, all other arguments are now out of alignment, including the 2 it read for the length..

Anyway, I created a typedef called fmr_size_t that replaces all length/size related parameters internally. This parameter should only be used in the context of a function parameter. size_t should be used when dealing with sizes outside of an FMR context.