Rationalize the different cline traceback options

[da-woods]

• If the user has specified the c_line_in_traceback=False option to the Cython compiler, then set CYTHON_CLINE_IN_TRACEBACK=0 to avoid generating all the code to print clines out.
• If the user has defined the C flag CYTHON_CLINE_IN_TRACEBACK=0 then skip recording the C line on an exception (since we know this is expensive in terms of code size, and we'll never actually use what we've recorded)

PR also includes commits from #6035 so merge that first.

This should probably be 3.1 only.

[scoder]

Here's a proposal that reduces the whole thing to checking CYTHON_CLINE_IN_TRACEBACK and generating different macros for both cases. I find it also simpler to read the different parts of the generated macro this way.

        code.putln("#ifndef CYTHON_CLINE_IN_TRACEBACK")
        code.putln(f"#define CYTHON_CLINE_IN_TRACEBACK {'1' if options.c_line_in_traceback else '0'}")
        code.putln("#endif")

        # Using "(void)cname" to prevent "unused" warnings.
        mark_errpos_code = (
            "#define __PYX_MARK_ERR_POS(f_index, lineno)  {"
            f" {Naming.filename_cname} = {Naming.filetable_cname}[f_index];"
            f" (void) {Naming.filename_cname};"
            f" {Naming.lineno_cname} = lineno;"
            f" (void) {Naming.lineno_cname};"
            "%s"  # for C line info
            "}"
        )
        cline_info = (
            f" {Naming.clineno_cname} = {Naming.line_c_macro};"
            f" (void) {Naming.clineno_cname}; "
        )

        code.putln("#if CYTHON_CLINE_IN_TRACEBACK")
        code.putln(mark_errpos_code % cline_info)
        code.putln("#else")
        code.putln(mark_errpos_code % "")
        code.putln("#endif")

[da-woods]

code.putln(f"#define CYTHON_CLINE_IN_TRACEBACK {'1' if options.c_line_in_traceback else '0'}")

This changes the current behaviour. Currently:

• CYTHON_CLINE_IN_TRACEBACK == 1 - always reports the c line in the traceback
• CYTHON_CLINE_IN_TRACEBACK == 0 - never reports the c line in the traceback
• CYTHON_CLINE_IN_TRACEBACK undefined - reports the c line only if cython_runtime.cline_in_traceback is set at runtime (and thus needs to have the information available.

I'm not against changing the current behaviour, but we should be aware we're doing it.

[scoder]

Argh, right. I forgot about the runtime configuration part. It's incredible in retrospect how much work we put into this feature. It certainly is a helpful debugging feature, but it should rather remain at that and not get in the way too much.

Second try:

        # Error handling and position macros.
        # Using "(void)cname" to prevent "unused" warnings.
        mark_errpos_code = (
            "#define __PYX_MARK_ERR_POS(f_index, lineno)  {"
            f" {Naming.filename_cname} = {Naming.filetable_cname}[f_index];"
            f" (void) {Naming.filename_cname};"
            f" {Naming.lineno_cname} = lineno;"
            f" (void) {Naming.lineno_cname};"
            "%s"  # for C line info
            "}"
        )
        cline_info = (
            f" {Naming.clineno_cname} = {Naming.line_c_macro};"
            f" (void) {Naming.clineno_cname}; "
        )

        if not options.c_line_in_traceback:
            code.putln("#ifndef CYTHON_CLINE_IN_TRACEBACK")
            code.putln("#define CYTHON_CLINE_IN_TRACEBACK  0")
            code.putln("#endif")
        code.putln("#if !defined(CYTHON_CLINE_IN_TRACEBACK) || CYTHON_CLINE_IN_TRACEBACK")
        code.putln(mark_errpos_code % cline_info)
        code.putln("#else")
        code.putln(mark_errpos_code % "")
        code.putln("#endif")

        code.putln("#define __PYX_ERR(f_index, lineno, Ln_error) \\")
        code.putln("    { __PYX_MARK_ERR_POS(f_index, lineno) goto Ln_error; }")

[da-woods]

That looks good with one small change: f" (void) {Naming.clineno_cname}; " should always be present since it avoids unused warnings.

I'll try that

[scoder]

~BTW, I also think it's fine to change the default in options.c_line_in_traceback to compiling this out. If users really want this feature, it's easy to enable with CFLAGS at compile time.~

EDIT: Well, again, the above doesn't work with the runtime config. But the following still holds:

In fact, I'd rather deprecate the Cython option(s) in 3.1 and let users decide about it entirely at C compile time. It's easy to set C macros, maybe even easier than setting a Cython option, for many users.

[scoder]

Right. That gives us this then:

        # Error handling and position macros.
        # Using "(void)cname" to prevent "unused" warnings.
        mark_errpos_code = (
            "#define __PYX_MARK_ERR_POS(f_index, lineno)  {"
            f" {Naming.filename_cname} = {Naming.filetable_cname}[f_index];"
            f" (void) {Naming.filename_cname};"
            f" {Naming.lineno_cname} = lineno;"
            f" (void) {Naming.lineno_cname};"
            "%s"  # for setting C line info
            f" (void) {Naming.clineno_cname}; "
            "}"
        )
        set_cline_info = (
            f" {Naming.clineno_cname} = {Naming.line_c_macro};"
        )

        if not options.c_line_in_traceback:
            code.putln("#ifndef CYTHON_CLINE_IN_TRACEBACK")
            code.putln(f"#define CYTHON_CLINE_IN_TRACEBACK  0")
            code.putln("#endif")
        code.putln("#if !defined(CYTHON_CLINE_IN_TRACEBACK) || CYTHON_CLINE_IN_TRACEBACK")
        code.putln(mark_errpos_code % set_cline_info)
        code.putln("#else")
        code.putln(mark_errpos_code % "")
        code.putln("#endif")

[da-woods]

How about we change to CYTHON_CLINE_IN_TRACEBACK being:

• 0 for disabled
• 1 for enabled
• -1 for runtime

(Defaulting to either 0 or -1 - I don't really mind). That means it can be controlled completely by the C define, and we can deprecate the options flag as you propose)

Edit: The only advantage in doing this is if we want to turn it to 0 by default though

[scoder]

• 0 for disabled
• 1 for enabled
• -1 for runtime

My first thought was -1, too, but I think 2 is better. It's like 1 (i.e. on) but more. :)

I also think that an explicit value is better than letting users figure out how to undefine it.

[scoder]

mention versions

We should still make it clear that the macro also works in older versions than 3.1. Users should generally migrate away from the options, not just for new Cython versions.

[matusvalo]

My last comment in this PR and it is just personal opinion. Please keep in mind that recently I have very limited free time so I did not went in deep so maybe I misunderstood something.

If I understand correctly default behavior (if you provide no macro and no runtime time option) is to have ~5% overhead present. In my humble opinion is that this is not what users want because:

1. C lines in traceback is not very known (I was not aware about it) since it was not documented how to show the C lines (nor option, nor runtime switch, nor cython CLI argument), hence I suppose it is not core feature of Cython
2. I saw huge care of users in compile times and binary size mainly in heavy users of Cython (see https://github.com/scikit-learn/scikit-learn/issues/27767)
3. Having it documented in Compilation userguide is great but the users needs to go down there and read it. Not sure how many of users will go there and maybe the users who knows cython already won't make it there anytime.

Having said all above, I think that we should generate binary without overhead and allow the users to add it only if they needs it. Hence the default behaviour should be equal to setting macro CYTHON_CLINE_IN_TRACEBACK=0.

[scoder]

default behaviour should be equal to setting macro CYTHON_CLINE_IN_TRACEBACK=0.

I agree with this. Let's see if we can find a way to make it easy for users to use the same setup in Cython 3.0 (and earlier?) and 3.1.

[da-woods]

default behaviour should be equal to setting macro CYTHON_CLINE_IN_TRACEBACK=0. I agree with this. Let's see if we can find a way to make it easy for users to use the same setup in Cython 3.0 (and earlier?) and 3.1.

The only way I can think to easily to that if to add a second macro (e.g. CYTHON_CLINE_IN_TRACEBACK_RUNTIME).

• In 3.1 it's defined then we use the current runtime behaviour; otherwise we use CYTHON_CLINE_IN_TRACEBACK which defaults to 0.
• In 3.0 it's just ignored, but that means we use the current runtime behaviour.

I don't really like it because it's two defines when it should be one. But I can't think of another way to achieve this.

[matusvalo]

I don't really like it because it's two defines when it should be one. But I can't think of another way to achieve this.

I agree with @da-woods . It depends what is less evil - current default overhead caused by C lines or 2 macros. I vote for having 2 macros. @scoder @da-woods what is your oppinion?

Maybe we can do following:

• CYTHON_CLINE_IN_TRACEBACK == 1 - always reports the c line in the traceback
• CYTHON_CLINE_IN_TRACEBACK == 0 or undefined - never reports the c line in the traceback
• CYTHON_CLINE_IN_TRACEBACK == 2 - reports the c line only if cython_runtime.cline_in_traceback is set at runtime (and thus needs to have the information available. Document that this is only for Cython 3.1 and later
• CYTHON_CLINE_IN_TRACEBACK_RUNTIME alias to CYTHON_CLINE_IN_TRACEBACK==2 with note that this is only for backward compatibility and use CYTHON_CLINE_IN_TRACEBACK == 2 whenever possible + it will be removed in future Cython versions.

It is more cumbersome but it enables us to clean it up in future releases...

[scoder]

I've proposed such a 2-macros implementation, making some handwavy assumptions.

• Most users do not need C lines in tracebacks for their user facing production releases and prefer smaller modules.
• If users want the feature, they'll probably prefer quickly and easily turning it on statically (e.g. for local debugging) over runtime checking, which involves multiple steps to set up.
• If users want the feature configurable at runtime, they can probably afford putting a bit of effort into it.

Thus, we should optimise for the "feature disabled" case, make it easy to enable the feature, and provide a way to configure it at runtime.

I think two separate macros model this quite ok.

[da-woods]

~It's more of a mess than I'm going it credit for (in Cython 3.0.x too).~

~It isn't something you can set to set in Cython.Compiler.Options despite kind of looking like it. It has to come from a command-line argument to cythonize.py, or possibly the no_c_in_traceback attribute on an extension.~

~I'm trying to write some tests and I can't actually work out how to set it from setup.py right now~

Worked it out, but it's not quite what I thought