Memos From the Cube

One of the types of messages which plagues AVRFreaks.net is the typical new user post – “I think I’ve found a compiler bug!”. These posts are inevitably solved by fixing the users code, and not the compiler toolchain; after all, if it can compile the Linux kernel correctly, it is unlikely – but of course, not impossible – that such simple code snippets would find a major code bug. The problem is in the coder and not the tool.

I too shared this sort of mild arrogance when I first started programming; I knew what I wanted, and I had written it down, so it must be the fault of the computer for not doing what I want correctly. Since then I’ve matured somewhat as a coder, and learned an invaluable lesson; be humble. Given a choice between myself being wrong and someone else, I now immediately suspect that the problem lies firmly in my own work. Usually, a bit of head scratching and debugging later and I prove myself right (or is that wrong?) and avoid looking like a complete muppet to the world at large. If that’s the only thing I can teach people I’d be happy, because learning to distrust one’s own code is the first step towards making better programs.

However, this is not such a case, and I am now mostly satisfied that GCC is at fault here, and not some bad code on my part.

Inside LUFA, I’ve added a set of functions designed to make the parsing of retrieved Configuration Descriptors – basically, a set of special binary tables in the device to describe the public interfaces the device exposes to the host – and easy affair. Since all standard descriptors share a common header, parsing the list of descriptors inside the Configuration Descriptor list becomes a process of walking the list, examining each descriptor in turn to decide if it is worth processing further or not.

One of the functions I’ve found to be most handy in this set is called USB_GetNextDescriptorComp(), which takes in the configuration descriptor list, the number of bytes remaining to be processed (so we can exit when the entire list has been processed) and a callback routine. For each descriptor in the list, the function will fire the callback, allowing the user code to determine if the descriptor matches some predetermined criteria. This pattern is used in all the Low Level API Host demos, such as the one here from the Low Level Mass Storage Host demo’s code. Note how just a small set of callback routines makes parsing the configuration descriptor a simple and reasonably concise affair.

However, the root of all the descriptor processing functions is a tiny inline function which does the actual increment of the descriptor pointer and decrement of the bytes remaining counter, called USB_GetNextDescriptor(). The code to this function is quite short, and the main reason it is inline:

static inline void USB_GetNextDescriptor(uint16_t* const BytesRem, void** CurrConfigLoc)
 {
    uint16_t CurrDescriptorSize = DESCRIPTOR_CAST(*CurrConfigLoc,
                                                  USB_Descriptor_Header_t).Size;
 
    *CurrConfigLoc += CurrDescriptorSize;
    *BytesRem      -= CurrDescriptorSize;
 }

Now one problem with this code is that the increment of the current descriptor pointer uses void* arithmetic, a no-no when LUFA is compiled in a C++ application. I use void pointers (or in this case, a pointer to a void pointer, since the function needs to be able to manipulate the pointer in the user application) in the routine since the descriptors don’t really have an applicable data type. The obvious way to solve this would be to cast the CurrConfigLoc variable to a uint8_t** when incrementing, so that the compiler is happy. Void pointer arithmetic is a GCC extension when the C language is used, and is interpreted as incrementing the pointer as if it pointed to a data type of size one, so this code should be exactly equivalent:

static inline void USB_GetNextDescriptor(uint16_t* const BytesRem, void** CurrConfigLoc)
 {
    uint16_t CurrDescriptorSize = DESCRIPTOR_CAST(*CurrConfigLoc,
                                                  USB_Descriptor_Header_t).Size;
 
    *((uint8_t**)CurrConfigLoc) += CurrDescriptorSize;
    *BytesRem      -= CurrDescriptorSize;
 }

And indeed, testing of a few demos proved it worked just fine. But further testing revealed that it broke the low level MouseHostWithParser demo. What was going on?

Looking at the compiled assembly, it was revealed that the act of adding the cast caused the compiler to bug out on some of the demos, creating faulty code. An analysis performed by Wek of AVRFreaks revealed that in some circumstances, the added cast caused the compiler to re-order the statements so that the callback function was invoked before the pointer addition, rather than after it.

The solution I found was to use a second, internal inline function, and cast the pointer when invoking the second function thusly:

static inline void USB_GetNextDescriptorST(uint16_t* const BytesRem, uint8_t** CurrConfigLoc)
 {
    uint16_t CurrDescriptorSize = DESCRIPTOR_CAST(*CurrConfigLoc,
                                                  USB_Descriptor_Header_t).Size;
 
    *CurrConfigLoc += CurrDescriptorSize;
    *BytesRem      -= CurrDescriptorSize;
 }
 
 static inline void USB_GetNextDescriptor(uint16_t* const BytesRem, uint8_t** CurrConfigLoc)
 {
    USB_GetNextDescriptorST(BytesRem, (uint8_t**)CurrConfigLoc);
 }

Voila, problem solved – or masked, at any rate. Generic pointer typing is preserved without breaking C++ compatibility.

What have I learned from all this? Be humble. But remember that occasionally, others make mistakes too.