c - correct struct element pointer arithmetic

Question

I'm newbie C programmer working on maintaining some legacy embedded C code that looks problematic. In the following snippets I have simplified:

UINT16 adcFunc(UINT8 adc, UINT8 channel)
{
    ADC_t* adc_ptr = (ADC_t*)(adc << 4);
    ADC_CH_t* adc_ch_ptr;
    adc_ch_ptr = (ADC_CH_t*)((UINT8*)&(adc_ptr->CH0) + sizeof(ADC_CH_t) * channel);
    ...
}

Where the structure definition is given as:

typedef struct ADC_struct
{
    ...
    register8_t reserved_0x1E;
    register8_t reserved_0x1F;
    ADC_CH_t CH0;  /* ADC Channel 0 */
    ADC_CH_t CH1;  /* ADC Channel 1 */
    ADC_CH_t CH2;  /* ADC Channel 2 */
    ADC_CH_t CH3;  /* ADC Channel 3 */
} ADC_t;

With the pointer size being 2 bytes and UINT8 represented as a typedef unsigned char. When linting the code, my linter reports back a warning

cast from UINT8* to ADC_CH_t* increases required alignment from 1 to 2 on the line

adc_ch_ptr = (ADC_CH_t*)((UINT8*)&(adc_ptr->CH0) + sizeof(ADC_CH_t) * channel);

The code is trying to calculate the correct offset into the struct for the channel pointer adc_ch_ptr (where channel is between 0 and 3) It looks like a strict aliasing violation to me and I removed the cast from (UINT8*) senselessly and it crashed the application.

Can anyone shed some light on how to correctly calculate the pointer to the correct channel without aliasing and padding/alignment issues?

Thanks

Why does the function accept an 8bit argument and assumes it to be a pointer? Does your compiler/platform support `void*` pointers? Why not use a switch instead of this pointer juggling? What is the function supposed to return? — wildplasser, May 03 '20 at 10:55
Please post the code that calls `adcFunc()`. `UINT8 adc ... ADC_t* adc_ptr = (ADC_t*)adc;` is suspicious. — chux - Reinstate Monica, May 03 '20 at 11:28
@wildplasser It does support void pointers and I have no idea why it was written like this because it's not my code. It's supposed to return an ADC reading on that channel pointer. — Nubcake, May 03 '20 at 20:59
@chux-ReinstateMonica I have updated the snippet of code. Sorry I had omitted the original because I thought it wasn't relevant. — Nubcake, May 03 '20 at 21:00

score 2 · Accepted Answer · answered May 03 '20 at 11:08

Avoid this pointer magic, and trust the compiler to understand the switch:

UINT16 adcFunc(UINT8 adc, UINT8 channel)
{
        /* this should be hidden inside a macro or an inline function*/
    ADC_t *adc_ptr = FIND_BASE_ADDRESS(adc);

    ADC_CH_t *adc_ch_ptr;

    switch (channel) {
    case 0: adc_ch_ptr = &adc_ptr->CH0; break;
    case 1: adc_ch_ptr = &adc_ptr->CH1; break;
    case 2: adc_ch_ptr = &adc_ptr->CH2; break;
    case 3: adc_ch_ptr = &adc_ptr->CH3; break;
        /* should not happen ... */
    default: return 0xffff;
        }

    /* do something with adc_ch_ptr ... */
    ...
    return something_usefull_here;
}

Eric Postpischil · Answer 2 · 2020-05-03T11:00:56.217

1

Two simple solutions are:

Ignore your “linter”. Leave the code as it is.
Change adc_ch_ptr = (ADC_CH_t*)((UINT8*)&(adc_ptr->CH0) + sizeof(ADC_CH_t) * channel); to adc_ch_ptr = &adc_ptr->CH0 + channel;.

Either of these relies on the address arithmetic working beyond what the C standard requires and the structure not having any weird (and unnecessary) padding. Slightly more complicated solutions using strictly conforming C code are below.

The changed code above simply treats the CH* members as if they were an array of ADC_CH_t; adding an integer channel to a pointer to the first element (with index 0) of an array produces a pointer to another element in the array (with index channel). The original code does the same arithmetic except in units of bytes instead of elements of type ADC_CH_t. It appears unnecessary to use bytes, as the arithmetic with elements should produce the same results. So it is unclear why the original author chose to use bytes, given that the resulting code is more cumbersome.

Two solutions that use strictly conforming C code are:

Use an array (defined here as a compound literal) to look up the desired address:

adc_ch_ptr = (ADC_CH_t *[]) {
        &adc_ptr->CH0, &adc_ptr->CH1, &adc_ptr->CH2, &adc_ptr->CH3,
    } [channel];

Use a switch:

switch (channel)
{
    case 0: adc_ch_ptr = &adc_ptr->CH0; break;
    case 1: adc_ch_ptr = &adc_ptr->CH1; break;
    case 2: adc_ch_ptr = &adc_ptr->CH2; break;
    case 3: adc_ch_ptr = &adc_ptr->CH3; break;
}

edited May 03 '20 at 11:00

answered May 03 '20 at 10:47

Eric Postpischil

195,579
13
168
312

Thanks for your answer. I have changed it to the switch statement but I'm just wondering about whether the original code is violating the strict aliasing rule? Could you tell me that? – Nubcake May 03 '20 at 21:02
@Nubcake: The original does not violate the “strict aliasing” rules, since it attempts to access `ADC_CH_t` objects using an `ADC_CH_t` type. However, it violates pointer arithmetic rules, since addition of an integer to a pointer to a single object (the member `CH0`) is defined only for integers 0 or 1. (Adding 1 produces a pointer to “just beyond” the single object, but it the behavior of using it to access memory is not defined by the C standard, so the expression is defined by the C standard only when `channel` is zero.) – Eric Postpischil May 03 '20 at 21:44
@EricPostpischil Apologies but I couldn't find a reference in the C standard (section 6.5.6 Additive operators) that pointer arithmetic is defined for integers 0 or 1 with single objects; Do you mean that only increment/decrement is allowed on a struct pointer and that this pointer arithmetic is undefined behaviour for `channel > 0`? – Nubcake May 03 '20 at 22:34
@Nubcake: Pointer arithmetic is defined only for results that point to an array element or one beyond the last element. E,g., for `int x[3]`, the result must be equivalent to `&x[0]`, `&x[1]`, `&x[2]`, or `&x[3]`. For this purpose, a single object is treated as an array of one element. So, for `int x`, the only defined results are for `&x+0` and `&x+1`. – Eric Postpischil May 03 '20 at 23:17
Thanks I've understood from your explanation now – Nubcake May 04 '20 at 15:59

c - correct struct element pointer arithmetic

2 Answers2