Avnu / libavtp

Open source implementation of Audio Video Transport Protocol (AVTP) specified in IEEE 1722-2016 spec.
BSD 3-Clause "New" or "Revised" License
62 stars 34 forks source link

Fix compilation with GCC 9 #27

Closed vcgomes closed 4 years ago

vcgomes commented 4 years ago

GCC 9 complains about the usage of unaligned access, the solution is add helpers that provide safe unaligned pointer access.

As all AVTP unaligned access deal with 32-bit Big Endian fields, the helpers also convert those values to/from the native CPU representation.

The code is based on code from the Linux kernel:

tools/include/linux/unaligned/packed_struct.h

Signed-off-by: Vinicius Costa Gomes vinicius.gomes@intel.com

edersondisouza commented 4 years ago

LGTM!

andrew-elder commented 4 years ago

Looks good to me too!

aguedes commented 4 years ago

Hi Vinicius, thanks for this PR. While the code also LGTM I’d like to clarify something.

My understanding is that the new -Waddress-of-packed-member check is about warning when we take the address of a packed member since accessing that address can lead to memory alignment faults in some architectures (https://reviews.llvm.org/D20561).

This is exactly what the current code does so we get the warning.

I’m under the impression that even with this PR the root cause of the warning is still there i.e. we still access a packed member by its address. The reason why gcc doesn’t complain anymore is because we trick the compiler. So if we ran this PR in one of those architectures we would see memory alignment faults, I believe.

Does the above make sense to you?

When I learned about this issue, the first fix that came to my mind was to read the PDU member value, modify it and write it back to the PDU struct. A quick look at Linux kernel git-log shows some fixes in that direction:

https://patchwork.kernel.org/patch/11324929/

https://patchwork.kernel.org/patch/11016353/

vcgomes commented 4 years ago

From what I understand this solution is right, in the sense that it's not only silencing the warning, but forcing the compiler to generate code that will work even the architecture doesn't support efficient unaligned access.

See here: https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt

And a sample of this on compiler explorer, my MIPS-fu is weak but it seems that it generates the right code:

https://godbolt.org/z/UBvQZe

But yeah, we can think of other solutions that avoid having to access unaligned pointers, but that requires re-thinking some of the libavtp internals.

andrew-elder commented 4 years ago

Some other AVB projects use inline functions llike https://github.com/audioscience/jdksavdecc-c/blob/master/include/jdksavdecc_util.h#L255 for all PDU pack/unpack operatoins.

aguedes commented 4 years ago

Thanks for the input folks.

@vcgomes, you are correct. Note the LWL and LWR instructions in the code you posted. They are the ones doing the trick!

I'm merging this now.