iamandi / nanopb

Automatically exported from code.google.com/p/nanopb
zlib License
0 stars 0 forks source link

Packed struct makes unaligned access #77

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. in protofiles add option: option (nanopb_fileopt).packed_struct = true;
2. define message where is optional field for example int
3. encode this message

Due to packed structs there is only one byte for bool ( has_proprety ) and then 
4 bytes for integer

Encoding follows sizes according to fields definitions.
Has_property has no problem, but then the function pb_enc_varint
is called with src pointer pointing to value, but this value is not at aligned 
address.
So code:
case 4: value = *(const uint32_t*)src; break;

Makes unaligned access.

There could be some correct memcpy instead.

What version of the product are you using? On what operating system?
nanopb-0.2.1, linux

Original issue reported on code.google.com by P.Sta...@gmail.com on 5 Jun 2013 at 2:45

GoogleCodeExporter commented 9 years ago
Will probably have to add some compile time option to define if the platform 
does not support unaligned accesses, so that there is no speed penalty for 
platforms that do.

Original comment by Petteri.Aimonen on 5 Jun 2013 at 2:52

GoogleCodeExporter commented 9 years ago
Hmm.. this would be quite nasty to fix. While pb_enc_varint() is simple enough, 
the pSize in encode_static_field() etc. would need all to be wrapped. And the 
same for the decoder side.

I'm not so sure if it is worth it to support packed_struct=true for 
architectures where unaligned accesses are not allowed. It's not many bytes of 
saving anyway.

Original comment by Petteri.Aimonen on 5 Jun 2013 at 7:08

GoogleCodeExporter commented 9 years ago
Maybe the simplest solution could be to add comment like:
When you use packed option then unaligned access may occur.
If it's problem is your solution simply do not add this option to proto files.

This option saves enough bytes when you have messages like capability message 
where is a lot of optional bools. But this could be also done by repeating enum.

Original comment by P.Sta...@gmail.com on 6 Jun 2013 at 8:34

GoogleCodeExporter commented 9 years ago
Yeah, it is probably best to just document this limitation.

More efficient storage of bools could be possible through some bitfield thing. 
That would be a separate feature.

Original comment by Petteri.Aimonen on 6 Jun 2013 at 8:37

GoogleCodeExporter commented 9 years ago
Documented by commit 6e9e5329278b04a8e76d63f06fed2f3bfa80e2f8

Original comment by Petteri.Aimonen on 6 Jul 2013 at 1:18

GoogleCodeExporter commented 9 years ago
Fix included in nanopb-0.2.2

Original comment by Petteri.Aimonen on 18 Aug 2013 at 7:17