address-of-packed-member & stringop truncation

[NaruFGT]

Fixed build errors relating to taking the address of struct car_key_format member identifier_list by allocating and copying member to a vector.

Fixed build errors relating to strncpy() of string literals. memcpy() is the appropriate function in this instance as there is no intention that these be null terminated.

[sas]

Are these warnings enabled by default? I see that you have fix the errors in the code, but if these warnings aren't enabled by default, more instances of this issue can come back later.

[NaruFGT]

On platforms with more recent gcc or clang versions, yes. Many of these compiler warnings are due to more inclusive default warnings. From gcc 9.1 release notes:

C family

• ...
• New warnings:
  • -Waddress-of-packed-member, enabled by default, warns about an unaligned pointer value from the address of a packed member of a struct or union.
• ...
• Enhancements to existing warnings:
  • -Wstringop-truncation also detects a subset of instances of reading past the end of unterminated constant character arrays

Now why gcc believes we are reading past the end of these 5 byte array literals is beyond me, but in any case, we know the contents of these arrays, we know how many bytes we wish to copy, memcpy() will suit our needs.

[NaruFGT]

If you're interested I can submit a PR which would check the compiler versions and enable these flags if the compiler doesn't enable them by default, but I'd imagine the previous versions of these compilers did not include this warning by default because this particular compiler feature wasn't stable or tested enough. That's just my speculation though.

[NaruFGT]

I've reviewed the documentation for GCC 4.8.5 which is what travis-ci is using and these particular warnings are not available for this version of GCC. I believe it will be best to rely on -Wall to provide these warnings should GCC/Clang support them.

[sas]

So the stringop-truncation changes look fine to me (if you want to put them in a separate PR, I can just approve and land it right away).

addressof-packed-member seems a bit more annoying because it's a warning that does not reflect a real bug (that I can see) in the code, at least for the architectures we currently target. Also, having this logic copy-pasted three times isn't great. I think a better fix would be to remove the packed attribute from these structures, and add a static_assert to guarantee a certain size. This won't make any functional difference but will suppress the warnings (because the structs won't be marked as packed anymore). See this: https://gist.github.com/sas/aff8c40a9094c05b321a3549de961857

[alexandre-janniaux]

Hi, is there any news on where this PR is going? I also have the build failing on -Werror=stringop-truncation and -Werror=addres-of-packed-member on archlinux.

[huytrvan]

Hi @sas, I can confirm that this pr fixes the build process on Ubuntu 20.10. It no longer gives the error that @NaruFGT mentioned. Seems to be the problem with importing C++ library.