IGRAPH_FINALLY() and IGRAPH_FINALLY_REAL()

[krlmlr]

CSan checks complain about our hack in IGRAPH_FINALLY() calling IGRAPH_FINALLY_REAL() with a cast function pointer: https://github.com/igraph/rigraph/actions/runs/12108577215/job/33756736066#step:6:10130 . My most recent submission got rejected from CRAN because of this.

Propose to only use IGRAPH_FINALLY_REAL() instead of IGRAPH_FINALLY() in the R interface, and create wrapper functions for each function called:

# Bad:
IGRAPH_FINALLY(igraph_vector_int_destroy, &news);

# Good:
IGRAPH_FINALLY_REAL(igraph_vector_int_destroy_pv, &news);

void igraph_vector_int_destroy_pv(void* v) {
  igraph_vector_int_destroy((magic_cast*)v);
}

(For some magic_cast we'd have to figure out. I can't quite parse igraph_vector.h . These days, I'd write a code generator and commit the generated code to version control, instead of this complex preprocessor logic because I find that easier to read.)

@szhorvat @ntamas: What do you think?

[szhorvat]

I think it is highly unreasonable to try to work around this warning, which can easily be disabled: -fno-sanitize=function. The workaround is extremely costly, complicated, and ultimately harmful (as in it suppresses complier warnings that caught real issues in the past).

We already knew that this warning existed, understand the reasons, and know that it is harmless.

I can also say right now that this is not the only UBSan warning we have.

[krlmlr]

Right, I hear you. How about:

    do { \
        /* the following branch makes the compiler check the compatibility of \
         * func and ptr to detect cases when we are accidentally invoking an \
         * incorrect destructor function with the pointer */ \
        if (0) { func(ptr); } \
        IGRAPH_FINALLY_REAL((func##_pv), (ptr)); \
    } while (0)

Note the ##_pv in the call. Could these variants be provided by our macros?

Arguing with CRAN is an uphill battle.

[szhorvat]

Let's hear from @ntamas as well.

At this point my opinion is that we need to try to talk reason into CRAN, but let's discuss the objective part below.

The change you propose (which preserves useful compiler warnings) is to duplicate every single destructor and provide a version (pv) that takes a void * pointer, then change IGRAPH_FINALLY to call the pv version.

• This is a lot of work and a significant change.
• This is a breaking change to igraph's error handling mechanism, and shouldn't be made in the 0.10 series. The promise right now is that if you have a destructor of the form void dest(something *), then you can use it with IGRAPH_FINALLY. This would need to be changed to "you must provide a wrapper to your destructor with a pv suffix".
• Making this change would make the igraph C library more complicated and less pleasant to use (arbitrary rule about providing pv destructor wrappers), potentially driving away users. It would also be more expensive to maintain.

An alternative (and also breaking) solution is to make destructors take a void * pointer and cast. This would be a harmful change that reduces safety and silences useful compiler warnings.

It seems that CRAN is starting to complain about UBSan warnings. It's important to note that:

• This is not the only UBSan warning in igraph at this moment. Expect more complaints.
• UBSan can produce many kinds of warnings. Which are enabled by default differ between Clang and GCC and keep changing between compiler versions. Some warnings that were on by default in the past got disabled later (for example floating point division by zero). There is no absolute benchmark about what is or isn't a relevant warning.
• There is no objective benefit from this change. It serves only to satisfy an arbitrarily imposed CRAN requirement.

Especially when working with limited resources, we can't afford to put multiple days of work into changes that bring no objective improvements, and are even potentially harmful (increased complexity, harder to use library, higher maintenance costs). I am leaving tomorrow morning and wouldn't even be able to look at this (many hours of work) for 2 weeks. I can write up an explanation that can be relayed to CRAN though.

[krlmlr]

The R package could define its own IGRAPH_FINALLY_PV(), along with the relevant functions.

I agree it's a moving target, but I'd rather just do the work (in the R package) than argue about it. It seems manageable. What is the alternative? Not have it on CRAN? Silence the test (by not running examples at all)?

[ntamas]

My two cents:

• I'd rather not let CRAN's idiosyncrasies determine how the C core of igraph is developed, so I'd rather not add the _pv functions to the C core
• On the other hand, @krlmlr as the maintainer of the R interface of igraph is free to do whatever workaround he needs to do in order to deal with CRAN as long as he is willing to put up with it. @szhorvat, I've dealt with CRAN before, and I can confirm that @krlmlr is right, it is unlikely that you can change CRAN policies single-handedly even if your arguments are valid -- there needs to be enough pressure from the R community in general.

So, if I were to call the shots, I'd probably say let's try to work around it in the R interface if we can limit the changes required to the R interface only, and we have the resources to deal with it. This is just an intrinsic part of the maintenance cost of the R interface, which is admittedly higher than what I need to invest in the Python interface to get the next version accepted in PyPI. You can try to argue, but be prepared for a long uphill battle with no guaranteed outcome and arbitrarily long delays for the next igraph release in CRAN.

[szhorvat]

The problem is that practically it's impossible to isolate these changes from the C core. We can say that we'll limit the changes to the R interface, but that doesn't eliminate the coupling between the two sides. This means that the workaround is still costly to implement, costly to maintain, and there will be more kinds of changes on the C core side that will necessitate a response on the R side, which is not ideal.

Anyway, let's make a decision.

It sounds like we sort-of have an agreement not to make changes in the C core. Correct me if I'm wrong.

@krlmlr, do you still want to do this workaround on the R side then, or do you want to try to convince CRAN first? If you will try to convince CRAN, do you need some explanatory text? If you intend to go with the code generation route, we can do something like trying to guarantee that all destructors have a predictable name, such as ..._destroy(). If you'd like to parse igraph.h, that should be done after running it through the preprocessor which expands all macros.

[szhorvat]

Final thought: This issue affects not only public destructors, but also private ones, so parsing headers will never be enough.

However, it occurs only during error handling, which is rare in the R test suite and in examples. Thus just to pass CRAN's tests we don't need to "fix" every single occurrence, affecting every private destructor.

[szhorvat]

FYI I just ran the C test suite with UBSan, which I do periodically, and fixed the single (harmless) warning that came up. CRAN is clearly enabling some non-default UBSan warnings though, and making UBSan warnings errors (instead of the default warning), so be prepared for much more ...

In particular, be prepared for complaints about conversions between float/integer, which is the hack we use in the R interface for checking whether a float value is representable as integer in a performant way. Fixing that issue will no doubt ave a performance cost once these checks start getting applied to vectors as well as scalar, something that I believe is necessary to do.

rigraph/src/rinterface_extra.c

Line 80 in 7414602

if (((igraph_integer_t) REAL(value)[0]) != REAL(value)[0]) {

[szhorvat]

You can also consider disabling this using a pragma, in a localized way. This is easy, and won't disable the warnings in places where the same issue may potentially be harmful.

[krlmlr]

Thanks. Agree to keep the changes local to the R package.

I can hand-roll the few _pv functions needed, but I wonder if they could be provided by the C core itself? That's the only request I'd make regarding the C core.

Re integerness check: is this really faster than calling floor() ?

CRAN checks for pragmas.

[szhorvat]

floor() tends to be extremely slow, as it involves a function call (yes, I did benchmark it). trunc() tends to be much better, as most compilers generate inline code for it. But it is not a complete solution. The best solution I can come up with that doesn't trigger UBSan is inlining a function like this, and feeding the output of trunc() to it:

https://github.com/igraph/igraph/blob/b25eda2dba6ccbcf5a47fe1cb77796a7f0f524d4/src/math/safe_intop.c#L106-L129

We use this function in a format reader, where it is not performance critical. I believe that using this will be significantly slower than the back-and-forth cast we use in R now, but I have not benchmarked this.

[krlmlr]

Earlier, I wondered if log2() can be of use, or if perhaps a compiler will be smart enough to generate code that amounts to comparing the exponent. Thanks for the investigation.

This is not for right now, it looks like we can push 2.1.2, but we'd want to address it for the subsequent version.

[szhorvat]

I can hand-roll the few _pv functions needed, but I wonder if they could be provided by the C core itself?

I'm not in favour of including them in the C core, but we can hear what @ntamas has to say.

If you decide to go ahead with the solution you propose, I suggest attempting to auto-generate these ONCE and include them in a single file that's checked into the rigraph repo. There may be issues with destructors declared as static—I'd need to check to be sure. When this is done, future updates can be done manually, which perhaps wouldn't be that much work after all.

I might be able to give the auto-generation a go at some point, but I'd do this in Mathematica (my comfort zone), so it'd be a one-time thing, leaving future updates to be done manually. The plan is to use #warning directives to output the names of functions that need a wrapper, try to parse their prototypes from a pre-processed igraph.h and determine the correct types to cast to.