NordicSemiconductor / zcbor

Low footprint C/C++ CBOR library and Python tool providing code generation from CDDL descriptions.
Apache License 2.0
114 stars 34 forks source link

zcbor.py: Fix decoding of unions with aliased values. #398

Closed petejohanson closed 7 months ago

petejohanson commented 7 months ago

I hit this issue when prototyping some usage of ZCBOR for ZMK. In particular, I had started with a union type like:

core_request = {
    (0: get_lock_state) /
    (1: unlock_request) /
    (2: lock_request)
}

get_lock_state = nil
unlock_request = nil
lock_request = nil

Without the change in this PR, this gets generated as:

static bool decode_core_request(
        zcbor_state_t *state, struct core_request *result)
{
    zcbor_log("%s\r\n", __func__);

    bool tmp_result = (((zcbor_map_start_decode(state) && ((((((zcbor_uint_decode(state, &(*result).union_choice, sizeof((*result).union_choice)))) && ((((((*result).union_choice == union_get_lock_state_m_c) && (((1)
    && 1)))
    || (((*result).union_choice == union_unlock_request_m_c) && (((1)
    && 1)))
    || (((*result).union_choice == union_lock_request_m_c) && (((1)
    && 1)))) || (zcbor_error(state, ZCBOR_ERR_WRONG_VALUE), false)))))) || (zcbor_list_map_end_force_decode(state), false)) && zcbor_map_end_decode(state))));

    if (!tmp_result) {
        zcbor_trace_file(state);
        zcbor_log("%s error: %s\r\n", __func__, zcbor_error_str(zcbor_peek_error(state)));
    } else {
        zcbor_log("%s success\r\n", __func__);
    }

    return tmp_result;
}

It never bothers dropping the expected 0xF6 value for null in the CBOR message.

If you replace the value in the CDDL with nil directly, current main does the right thing:

core_request = {
    (0: nil) /
    (1: unlock_request) /
    (2: lock_request)
}

get_lock_state = nil
unlock_request = nil
lock_request = nil

generates:

static bool decode_core_request(
        zcbor_state_t *state, struct core_request *result)
{
    zcbor_log("%s\r\n", __func__);

    bool tmp_result = (((zcbor_map_start_decode(state) && ((((((zcbor_uint_decode(state, &(*result).union_choice, sizeof((*result).union_choice)))) && ((((((*result).union_choice == union_uint0nil_c) && (((1)
    && (zcbor_nil_expect(state, NULL)))))
    || (((*result).union_choice == union_unlock_request_m_c) && (((1)
    && 1)))
    || (((*result).union_choice == union_lock_request_m_c) && (((1)
    && 1)))) || (zcbor_error(state, ZCBOR_ERR_WRONG_VALUE), false)))))) || (zcbor_list_map_end_force_decode(state), false)) && zcbor_map_end_decode(state))));

    if (!tmp_result) {
        zcbor_trace_file(state);
        zcbor_log("%s error: %s\r\n", __func__, zcbor_error_str(zcbor_peek_error(state)));
    } else {
        zcbor_log("%s success\r\n", __func__);
    }

    return tmp_result;
}

You can see the added zcbor_nil_expect.

I tracked this down to the call in single_func_prim when we have an OTHER value. When this happens, it forwards on the DROP value for union_int, but that's different from the handling in repeated_xcode which only passes down the union_int for the specific types that need it.

This one line change seems to fix my specific use case, but I'm definitely not super familiar with the code, so there may be a better fix than this, for sure. With the code change, the generated code properly calls zcbor_nil_expect in all the branches even with type aliases used.

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

oyvindronningstad commented 7 months ago

Thanks a lot for finding and debugging this! I looked into it and found the issue went a bit deeper so I made a new PR for the fix and regression tests. Having your report and fix made it so much easier to find the problem! I credited you in the commit message if that's ok. https://github.com/NordicSemiconductor/zcbor/pull/405

petejohanson commented 7 months ago

Thanks a lot for finding and debugging this! I looked into it and found the issue went a bit deeper so I made a new PR for the fix and regression tests. Having your report and fix made it so much easier to find the problem! I credited you in the commit message if that's ok. https://github.com/NordicSemiconductor/zcbor/pull/405

Glad to help. I figured there must be a deeper/cleaner fix here, and I also failed in providing tests!

Thanks for fixing this.