Alignment violation fix in emitbc #747
Conversation
dummy_data field is accessed as uint value (e.g. in emit_write_bytecode_byte_ptr), but is not aligned as such, which causes bus errors or incorrect behavior on any arch requiring strictly aligned data (ARM pre-v7, MIPS, etc, etc).
|
Apparently, should be moved to the very end of structure to preserve packed structure size. |
|
Nice catch. But I would prefer not to rely on having dummy_data aligned to a machine word within the emit_t struct (eg on 64 bit is it still aligned to a machine word when you put it at the end?). Could put dummy_data at the very start of the struct, that would guarantee allignment. Or, make dummy_data have type mp_uint_t (wastes a few bytes). Or, have a function |
|
Since its declared as an array of bytes, it's only guaranteed to have byte alignment. You can use an anonymous union to force the alignment: union {
byte dummy_data[DUMMY_DATA_SIZE];
uint alignment;
};Here's my little test program: #include <stdio.h>
#include <stdint.h>
#include <stddef.h>
typedef struct {
uint8_t a;
union {
uint8_t b[12];
unsigned int c;
};
} foo_t;
int main(int argc, char **argv) {
printf("Offset of a = %lu\n", offsetof(foo_t, a));
printf("Offset of b = %lu\n", offsetof(foo_t, b));
printf("Offset of c = %lu\n", offsetof(foo_t, c));
return 0;
}and the output: |
|
A union, I like it. |
Well, magic is not in being put at the end, but in being put after word-sized field, that surely guarantees alignment. Putting at the beginning wastes good slot for it - the most accessed field should be there, because archs which don't have index+offset addressing will take more cycles to calc address of any other field. mp_uint_t for dummy_data is what first version of the patch did. All other solutions add complications/syntactic noise IMHO. So, let me merge this as is, and please apply more changes if you think they're needed. I'd rather suggest what to do about "pass" and "last_emit_was_return_value". They naturally would be moved after dummy_data to get back to the original struct sizeof, but I didn't do that exactly of the concern that "pass" is a frequently-accessed field. |
Okay, no problem. Just note that |
add storage.erase_filesystem() to erase and reformat CIRCUITPY
I caught this when running on armv5. There specifically alignment violation didn't cause any segfault, just using non-aligned word pointer masks 2 lowest bits. In this case, uint assignment to dummy_data actually overwrote ->pass. It was a bit of chore to debug ;-). And I wonder, if we have more such cases...