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" 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 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?
• What are Aggregates and PODs and how/why are they special?

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:

#include <type_traits>
// This should fail, per the GCC warning
static_assert(std::is_trivially_copyable<DmacDescriptor     >::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<DMAC_BTCTRL_Type   >::value);
static_assert(std::is_trivially_copyable<DMAC_BTCNT_Type    >::value);
static_assert(std::is_trivially_copyable<DMAC_SRCADDR_Type  >::value);
static_assert(std::is_trivially_copyable<DMAC_DSTADDR_Type  >::value);
static_assert(std::is_trivially_copyable<DMAC_DESCADDR_Type >::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 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:

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! 👍