@yoctopuce suggested we test MICROPY_OBJ_REPR_C and MICROPY_LONGINT_IMPL_LONGLONG together, which makes sense - and I don't think we have CI coverage for the former at the moment.

@dpgeorge what do you think?

Yes, that's a very good idea to test MICROPY_OBJ_REPR_C in CI.

It's enabled on esp8266 and stm32 Arduino boards, so it does get tested for a release. But would be great to test it in CI.

Enabling representation C in this new longlong variant is a good idea!

MICROPY_OBJ_REPR_C seems to need more work than just enabling it for this config, so I'll look at that for a follow-up PR.

Yeah, it's a bit of a nuisance to need to include this header... it sometimes doesn't exist on embedded systems.

At the very least I'd suggest moving this include to within the MICROPY_LONGINT_IMPL == MICROPY_LONGINT_IMPL_LONGLONG guard, so it's only included if needed (because this file will be compiled on all systems regardless of the config).

In the long run, would be best to remove dependency on errno, but I'm not sure it's worth the effort given this config is likely not used much.

I've moved it down behind the preprocessor guard.

I did start writing a version that doesn't depend on strtoll, using the approach @yoctopuce suggested for mp_parse_num_integer() to handle 64-bit. It has potential to work quite nicely, but it wasn't particularly simple either so I shelved it for now.

Here is my version, in case it helps:

    const byte* restrict str = (const byte*)*str_;
    const byte* restrict top = str + len;
    long long int_val = 0;
    for (; str < top; str++) {
        // get next digit as a value
        mp_uint_t dig = *str;
        if ('0' <= dig && dig <= '9') {
            dig -= '0';
        } else if (dig == '_') {
            continue;
        } else {
            dig |= 0x20; // make digit lower-case
            if ('a' <= dig && dig <= 'z') {
                dig -= 'a' - 10;
            } else {
                // unknown character
                break;
            }
        }
        if (dig >= (mp_uint_t)base) {
            break;
        }

        // add next digit
        int_val = int_val * base + dig;
    }

    // negate value if needed
    if (neg) {
        int_val = -int_val;
    }
    mp_obj_t result = mp_obj_new_int_from_ll(int_val);
    *str_ = (char*)str;

I got inspired to go and finish the version of this I was working on, have updated this PR with the result.

I think this needs to be wrapped in #if defined __has_builtin. See py/misc.h and https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fbuiltin.html

Or follow lib/cmsis/inc/cmsis_gcc.h:

#ifndef __has_builtin
  #define __has_builtin(x) (0)
#endif

I also hit that __has_builtin issue when integrating this commit in my dev branch, and as __has_builtin() is sometime undefined on compilers that do have support for __builtin_*_overflow, I ended up using this code:

#if defined(_MSC_VER)
#define ENABLE_INTSAFE_SIGNED_FUNCTIONS
#include <intsafe.h>
#define add_overflow(lhs,rhs,res)   LongLongAdd(lhs, rhs, res)
#define sub_overflow(lhs,rhs,res)   LongLongSub(lhs, rhs, res)
#define mul_overflow(lhs,rhs,res)   LongLongMult(lhs,rhs,res)
#elif defined(__EMSCRIPTEN__ ) || (defined(__GNUC__) && __GNUC__ >= 5)
#define add_overflow(lhs,rhs,res)   __builtin_add_overflow(lhs, rhs, res)
#define sub_overflow(lhs,rhs,res)   __builtin_sub_overflow(lhs, rhs, res)
#define mul_overflow(lhs,rhs,res)   __builtin_mul_overflow(lhs, rhs, res)
#endif

#if !defined(add_overflow)
#include <limits.h>

__inline bool add_overflow(long long lhs, long long rhs, long long *result) {
    // Check for overflow
    if ((rhs > 0) && (lhs > LLONG_MAX - rhs)) {
        return 1; // Overflow
    }
    if ((rhs < 0) && (lhs < LLONG_MIN - rhs)) {
        return 1; // Overflow
    }
    *result = lhs + rhs;
    return 0; // No overflow
}

__inline bool sub_overflow(long long lhs, long long rhs, long long *result) {
    // Check for overflow
    if ((rhs > 0) && (lhs < LLONG_MIN + rhs)) {
        return 1; // Overflow
    }
    if ((rhs < 0) && (lhs > LLONG_MAX + rhs)) {
        return 1; // Overflow
    }
    *result = lhs - rhs;
    return 0; // No overflow
}

__inline bool mul_overflow(long long lhs, long long rhs, long long * result)
{
    if (lhs > 0) {
        if (rhs > 0) {
            if (lhs > (LLONG_MAX / rhs)) {
                return 1; // Overflow
            }
        } else {
            if (rhs < (LLONG_MIN / lhs)) {
                return 1; // Overflow
            }
        }
    } else {
        if (rhs > 0) {
            if (lhs < (LLONG_MIN / rhs)) {
                return 1; // Overflow
            }
        } else {
            if ((lhs != 0) && (rhs <= (LLONG_MAX / lhs))) {
                return 1; // Overflow
            }
        }
    }
    *result = lhs * rhs;
    return 0; // No overflow
}
#endif

My embedded platforms are all using GCC >= 5, so the bigger+slower inline code only ends up in OS-based test builds.

Impressive! @dpgeorge do you think it's worth pulling any/all of this into MP, or stick with __has_builtin and fallback to not checking overflow? (__has_builtin is GCC 10 and up, first released 2020.)

(I've added the check for defined __has_builtin already.)

If we rely only on __has_builtin, we are going to significantly increase code size for all compiler version between 2015 ( GCC 5+) and 2020 (GCC 10+).

I think the impact is not that bad. If I build the Zephyr nucleo_wb55rg board with this branch (and gcc 12) I get 242792 bytes flash size.

If I manually patch the three #if __has_builtin(*overflow) lines to #if 0 then I get 242800 bytes flash size.

Admittedly, there is now no overflow check for addition and subtraction and it's possible GCC 12 has peephole optimised the manual mp_mul_ll_overflow function to an overflow check in a way that older GCC can't detect.

It is a bit odd that GCC version determines whether or not an overflow is an exception, though. I'll see how hard it is to patch around that further.

I've updated the implementation so the overflow checks are consistent regardless of whether __has_builtin(*overflow) is true or not. Based on your code, so thanks for that! I think my version of the manual implementations are all sound...

The Zephyr build still only goes up by 8 bytes in code size if I swap from the built-in overflow checked ops to the manual ones, though. Even on GCC 5.x the difference in size seems neglible: https://gcc.godbolt.org/z/jhhn16dW6 (One key difference may be that the builtins return a correct "infinite precision" result even in case of overflow, whereas the manual functions can fall back on UB for overflow.)

On balance I'd rather keep the checks simple and fallback to the manual implementations rather than keep adding per-compiler tweaks here. Unless there's a case where it really does balloon out the code size.

@yoctopuce I'm sorry, I missed your reply from 3 days ago - GitHub hadn't loaded it into the tab I still had open so I just saw it now.

That's interesting you see such a big difference by comparison to the things I checked against. Can put in the suggested extra check for built-ins, then.

@yoctopuce Added the explicit GCC version check. Curious how the version that's there now compares to your tests. (It's still possible I'm doing something differently or incorrectly in a way that's impacting the code size difference.)

Argh, clearly a case of "more haste less speed" today.

• The Zephyr port code size doesn't change by much because it isn't using MICROPY_LONGINT_IMPL_LONG_LONG. I don't know why I thought it did (or why a no-op source change causes an 8 byte difference in code size).
• Updated the PR again to rely on the behaviour that an undefined macro has value 0, so we don't need to recursively expand a defined clause.