gh-128394: Parse int for lltrace

[WolframAlph]

• Issue: Parse int for lltrace #128394

[ZeroIntensity]

Thanks for doing this. atoi() is quite broken and we definitely shouldn't use it here (for example, inputting something outside the number range is UB). You probably want something like PyOS_strtol instead.

[WolframAlph]

@ZeroIntensity

atoi() is quite broken and we definitely shouldn't use it here (for example, inputting something outside the number range is UB).

1. That's true, but my understanding is that people running locally debug builds know what they are doing and trying to deliberately cause UB does not make sense.
2. strtol and specifically PyOS_strtol is better option of course but that requires to check errno if we intend to use it properly which seems like overkill to me in this scenario. Would it make sense to use it without checking for errno just to rule out UB and keep things simple? WDYT

[ZeroIntensity]

You can probably just fatal error if it fails.

[WolframAlph]

@ZeroIntensity since PyOS_strtol returns long and lltrace is int, we need a bit more code to handle things correctly. Im thinking about creating standalone function for retrieving lltrace related env variables something like:

static inline int get_lltrace(const char* env_var) {
    int lltrace = 0;
    char* value = Py_GETENV(env_var);
    if (value != NULL) {
        long result = PyOS_strtol(value, NULL, 10);
        if (errno == ERANGE || result < 0 || result > INT_MAX) {
            char buff[100];
            PyOS_snprintf(buff, sizeof(buff), "Ivalid value for: %s", env_var);
            Py_FatalError(buff);
        }
        lltrace = (int)result;
    }
    return lltrace;
}

WDYT? The only question is where to put it

[ZeroIntensity]

Sure, that works. Anywhere in the file is fine (prefereably near the lltrace code).

[Fidget-Spinner]

I'm of the opinion that the added complexity to parse it isn't worth it. IIRC that comment was added by Guido, but I've never found a use for actually parsing an int? We never use any levels above 5 anyways. So it would only matter if we were to use two digit numerals (10 and onwards), which I don't see us using anytime soon.

I'd rather we just remove the comment. If we need to parse ints in the future, we can just do it then rather than preparing for something which might never happen.

[WolframAlph]

@Fidget-Spinner what about defining this logic somewhere else? #128395 (comment). This piece of logic is used across multiple files and copied over every time someone needs it. Might be handy and cleaner IMO

[Fidget-Spinner]

@Fidget-Spinner what about defining this logic somewhere else? #128395 (comment). This piece of logic is used across multiple files and copied over every time someone needs it. Might be handy and cleaner IMO

That would be nice. Thanks.

[gvanrossum]

I'm sorry, this does not deserve to be fixed. For an explanation see the issue.