Use of undefined behavior

[henrygab]

FAILING TO FIX THIS RESULTS IN WARNINGS OF POTENTIAL UNDEFINED BEHAVIOR.

This should not be worked around by casting to void*.

~The open question: What causes DmacDescriptor to not be considered trivially copyable?~

GCC warning of undefined behavior

``` ...\libraries\SPI\SPI.cpp: In member function 'void SPIClass::dmaAllocate()': ...\libraries\SPI\SPI.cpp: Line 338 Char 37 warning: 'void* memcpy(void*, const void*, size_t)' writing to an object of type 'struct DmacDescriptor' with no trivial copy-assignment [-Wclass-memaccess] 338 | sizeof(DmacDescriptor)); | ^ ...\libraries\SPI\SPI.cpp: Line 342 Char 37 warning: 'void* memcpy(void*, const void*, size_t)' writing to an object of type 'struct DmacDescriptor' with no trivial copy-assignment [-Wclass-memaccess] 342 | sizeof(DmacDescriptor)); | ^ In file included from ...\packages\arduino\tools\CMSIS-Atmel\1.2.0/CMSIS/Device/ATMEL/samd21/include/samd21g18a.h:253, from ...\packages\arduino\tools\CMSIS-Atmel\1.2.0/CMSIS/Device/ATMEL/samd21/include/samd21.h:69, from ...\packages\arduino\tools\CMSIS-Atmel\1.2.0/CMSIS/Device/ATMEL/samd.h:105, from ...\packages\arduino\tools\CMSIS-Atmel\1.2.0/CMSIS/Device/ATMEL/sam.h:540, from ...\cores\arduino/Arduino.h:48, from ...\libraries\SPI\SPI.h:23, from ...\libraries\SPI\SPI.cpp:20: ...\packages\arduino\tools\CMSIS-Atmel\1.2.0/CMSIS/Device/ATMEL/samd21/include/component/dmac.h:1070:16: note: 'struct DmacDescriptor' declared here 1070 | typedef struct { | ^ ```

[henrygab]

EXPANDABLE .. but collapsed so thread doesn't get too long.

Why this matters:

GCC docs for "[class-memaccess](https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#C_002b_002b-Dialect-Options )" explain why this is VERY BAD: Warn when the destination of a call to a raw memory function such as memset or memcpy is an object of class type, and when writing into such an object might: * bypass the class non-trivial or deleted constructor or copy assignment, * violate const-correctness or encapsulation, or * corrupt virtual table pointers. Modifying the representation of such objects may violate invariants maintained by member functions of the class. For example, the call to memset below is undefined because it modifies a non-trivial class object and is, therefore, diagnosed. The safe way to either initialize or clear the storage of objects of such types is by using the appropriate constructor or assignment operator, if one is available. Very few objects are actually trivially copyable. One [good source](https://en.cppreference.com/w/cpp/types/is_trivially_copyable) states: The only trivially copyable types are scalar types, trivially copyable classes, and arrays of such types/classes (possibly cv-qualified). This is covered in some detail by the following two SO threads: * [Can I memcpy() any type which has a trivial destructor?](https://stackoverflow.com/questions/11979153/can-i-memcpy-any-type-which-has-a-trivial-destructor) * [What are Aggregates and PODs and how/why are they special?](https://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-how-why-are-they-special/7189821#7189821)

Therefore, it's very important to have a correct fix, not just hide this warning.

Next steps might be to look into: std::is_trivially_copyable<typeof(DmacDescriptor)>::value

Presuming this is false, why is it false, and what can be done to ensure only defined compiler behavior is used?

[ladyada]

ok do you have a PR fix suggestion?

[henrygab]

Not yet, but I plan to.

Outdated (click to expand)

I'm curious, especially because this is a type of problem that is essentially impossible to track down, after it manifests / crashes. My first instinct was to just check the types traits on the struct and its members: ```C++ #include // This should fail, per the GCC warning static_assert(std::is_trivially_copyable::value); // DmacDescriptor itself is trivial, but is made of five other structures // Thus, at least one of these should also fail, helping to narrow the cause static_assert(std::is_trivially_copyable::value); static_assert(std::is_trivially_copyable::value); static_assert(std::is_trivially_copyable::value); static_assert(std::is_trivially_copyable::value); static_assert(std::is_trivially_copyable::value); ``` Oddly, **_none of the above static asserts triggered_.** Curious....

[henrygab]

Root Cause

The `-Wclass-memaccess` diagnostics appear to have been added in GCC 8.1. By experimenting with a re-created smaller code base, [godbolt](https://godbolt.org/z/acjMYjaqc) helped me understand this. In particular, by attempting to assign the structure directly (allowing the compiler to figure out the safe/optimized copy or move to use), the errors become clear. (Search for "XYZZY" for line to uncomment.) As the godbolt link shows, this only occurs where a field in the structure is `volatile` qualified. As far back as GCC 5.4.1, the compiler **_properly refuses_** to generate a copy constructor for `DmacDescriptor`.

Technical details on why copy constructor not available

Taking `DMA_BTCTRL_Type` as an example, the compiler auto-generates the following signatures: ```C++ DMA_BTCTRL_Type(); // default constructor ~DMA_BTCTRL_Type(); // default destructor DMA_BTCTRL_Type(const DMA_BTCTRL_Type& other); // copy constructor DMA_BTCTRL_Type(DMA_BTCTRL_Type&& other); // move constructor DMA_BTCTRL_Type& operator=(const DMA_BTCTRL_Type& other); // copy assignment DMA_BTCTRL_Type& operator=(DMA_BTCTRL_Type&& other); // move assignment ``` However, because the field is declared as volatile, the type of the parameter available to the compiler is: `const volatile DMA_BTCTRL_Type` The compiler cannot bind that to the copy assignment operator: `error: binding 'const volatile DMAC_BTCTRL_Type' to reference of type 'const DMAC_BTCTRL_Type&' discards qualifiers` The compiler cannot bind that to the copy assignment operator: `error: cannot bind 'const volatile DMAC_BTCTRL_Type' lvalue to 'DMAC_BTCTRL_Type&&'` Therefore, the compiler cannot automatically generate a copy constructor, because it cannot find a matching function to copy it's constituent fields.

---

Because there is not copy constructor, it appears the code just used `memcpy()`. The difference now, is that GCC has a diagnostic to flag these uses of `memcpy()`, as they could be a source of bugs that are extremely difficult to otherwise root-cause.

---

Starting in GCC 8, GCC added a diagnostic that flags the use of `memcpy()` on classes that are not trivially copy-constructed. This makes sense, as using `memcpy()` on these classes is a source of bugs that are **_extremely_** difficult to root-cause. Therefore, ignoring this warning cannot be done lightly.

Here, the use of memcpy() appears safe...

Here, the descriptors are allocated just a few lines earlier using standard `malloc()`. This means that the hardware is *not* currently using them, and thus the volatile qualifiers for the fields of `DmacDescriptor` can be safely ignored. When doing so, the structure would be trivially copy assignable, which means it is also safe to use memcpy().

---

See PR #290.

[henrygab]

@ladyada -- PR is ready. Explanation of root cause and why the fix is correct in prior comment. Hope that helps! 👍