Add scip::ObjExprhdlr class to objscip

[kkofler]

The intent is to be able to use SWIG director classes to implement expression handlers through bindings, in particular, to add support for expression handlers to JSCIP. That needs the C++ class to build upon.

src/objscip/objexprhdlr.h, src/objscip/objexprhdlr.cpp: New files. C++ wrapper for expression handlers. The files contain the scip::ObjExprhdlr class and the functions SCIPincludeObjExprhdlr, SCIPfindObjExprhdlr, and SCIPgetObjExprhdlr.

src/CMakeLists.txt (objscipsources): Add objscip/objexprhdlr.cpp.
(objscipheaders): Add objscip/objexprhdlr.h.

[svigerske]

@kkofler Are you ok with the current changes to your changes?

[kkofler]

Yes, looks good now. Thank you for your improvements to my contribution.

[svigerske]

Thank you for the contribution!

[kkofler]

Unfortunately, now that I am finally far enough with my Java bindings to be able to really test this, I have found an annoying issue, but I am not sure about the proper fix: The problem is: The Java ExpressionHandler class wants to call SCIPincludeObjExprhdlr in the constructor (in any constructor, even the copy constructor) so that it can remember the corresponding C SCIP_EXPRHDLR * pointer. As a result, the implementation of the clone operation (called by the ObjExprhdlr implementation of copyhdlr) is also going to indirectly call SCIPincludeObjExprhdlr from the Java code.

However, the C++ code also tries to directly call SCIPincludeObjExprhdlr in copyhdlr:

      /* call include method of expression handler object */
      SCIP_CALL( SCIPincludeObjExprhdlr(scip, newobjexprhdlr, TRUE) );

That was not my idea: At least ObjConshdlr does the exact same thing, I just adapted that code here.

As a result, the expression handler is going to be included twice, with both (duplicate) SCIP_EXPRHDLRs pointing to the same ObjExprhdlr. This eventually results in a double-free, and then all hell may break loose. (In my tests, I ended up running into a mysterious assertion failure that turned out to be caused by memory corruption: the second free was use-after-free-ing a SCIP * pointer that was already overwritten, and complaining that it did not match the passed SCIP *. But the behavior is undefined, so it can fail in many other ways.)

I tried simply commenting out that SCIP_CALL( SCIPincludeObjExprhdlr(scip, newobjexprhdlr, TRUE) ); line in objexprhdlr.cpp (pushing the reponsibility of calling that method down to the clone implementation) and that fixed it for me. However, then we should also change that behavior in objconshdlr.cpp (and any other objscip classes that do the same, if any) for consistency.

Fixing this in the JSCIPOpt Java code is not easy, also because, as I already mentioned, SCIPincludeObjExprhdlr is what gives me the SCIP_EXPRHDLR * pointer.

So I am not sure what the proper fix is here.

[svigerske]

The Java ExpressionHandler class wants to call SCIPincludeObjExprhdlr in the constructor (in any constructor,

Could you remove that it calls SCIPincludeObjExprhdlr() in any constructor?

Looking at objconshdlr and ConshdlrSubtour in the TSP example, there are not that many calls to SCIPincludeObjConshdlr. It's in conshdlrCopyObj only.

[kkofler]

Looking at objconshdlr and ConshdlrSubtour in the TSP example, there are not that many calls to SCIPincludeObjConshdlr. It's in conshdlrCopyObj only.

It is actually also called in the TSP example's cppmain.cpp in runSCIP. Basically, it must be called from somewhere so that the constraint handler can actually be used.

That said:

Could you remove that it calls SCIPincludeObjExprhdlr() in any constructor?

I originally thought that would not be feasible, but I now think I have a way to do that in the Java binding without needing 2 separate ExpressionHandler wrapper classes (one for the C++ scip::ObjExprHdlr* and one for the C SCIP_EXPRHDLR*): Basically, I can leave the ExpressionHandler's internal SCIP_EXPRHDLR* initially set to nullptr (actually, since the wrapper class ExpressionHandler is in Java, the internal SWIGTYPE_p_SCIP_Exprhdlr set to null) and let the SCIPincludeObjExprhdlr wrapper method set it. Anything trying to use the SCIP_EXPRHDLR getters (once I actually wrap them in the Java class, which is not even implemented yet) before the pointer is set will then fail, but that is expected. (The getters will also fail on the cloned ExpressionHandler class from the exprhdlrCopyhdlr operation, but nobody should be running the getters on that one from Java anyway. The getters only provide statistics and are not useful to implement the ExpressionHandler callback methods.)

That does mean, though, that a SCIPincludeObjExprhdlr equivalent has to be explicitly called, whereas the current Java bindings do it automatically (in the constructor, which needs the Scip object anyway). Though that matches how the C and C++ interfaces work.

[kkofler]

Actually, now that I have tried to implement that and looked closer, I do not think the above is a good idea, unfortunately. The Java ExpressionHandler class needs the C SCIP_EXPRHDLR* pointer not just to implement the statistics getters, but also to implement createExpression (actually, the protected createExpressionInternal method that is supposed to be called by the subclass's createExpression), because SCIPcreateExpr needs that pointer. That, in turn, may be needed to implement some callbacks, at least parse (but that gets the SCIP_EXPRHDLR* pointer passed explicitly, and it is also unlikely to be called in a sub-SCIP, so that is not the real problem) and simplify (if I want to return an expression of the same type, but simpler). So it is a problem that the cloned ExpressionHandler is missing this pointer because SCIPincludeObjExprhdlr was called from C++ (ObjExprHdlr::exprhdlrCopyObj) rather than from Java (where the pointer was stored).

So I do really want to always call SCIPincludeObjExprhdlr in the Java wrapper class constructor, which in turn means either ObjExprHdlr::exprhdlrCopyObj needs to stop calling it (as I did in my PoC #159) or we need to allow and filter redundant calls to it.

[svigerske]

In the C codes, we also have a SCIPcreateExpr* find the expression handler via SCIPfindExprhdlr, if it isn't one of the exprhdlrs where a dedicated core function is available, e.g.,

   SCIP_CALL( SCIPcreateExpr(scip, expr, SCIPfindExprhdlr(scip, COSEXPRHDLR_NAME), NULL, 1, &child, ownercreate,
            ownercreatedata) );

for SCIPcreateExprCos.

So couldn't the Java wrapper also use SCIPfindExprhdlr() to find the SCIP_EXPRHDLR*? It could do this only once and store it in the wrapper object, I suppose.

[kkofler]

So couldn't the Java wrapper also use SCIPfindExprhdlr() to find the SCIP_EXPRHDLR*?

That sounds workable, yes. I am giving that a try now.

[kkofler]

This is now implemented in JSCIPOpt (exprhdlr branch):

• ExpressionHandler constructors no longer automatically call SCIPincludeObjExprhdlr. Instead, there is an include() method that must be called explicitly.
• The internal SCIP_EXPRHDLR* is set by include(). Before, it is null. Methods that need the pointer now all use the getPtr() getter instead of accessing the field directly. If getPtr() is called with a null SCIP_EXPRHDLR*, it will try to look up the pointer using SCIPfindExprhdlr, in case SCIPincludeObjExprhdlr was called from C++. If that also returns null, an exception is thrown, telling the user that include() must be called first. (I could try to be extra smart and automatically call include() in that case, so include() would normally end up getting called when the first expression using the expression handler is constructed, but I am currently not convinced that that kind of automagic is more helpful than confusing.)
• The proposed patch from PR WIP/RFC: Do not call SCIPincludeObjExprhdlr in exprCopyhdlrObj #159 is obsolete and no longer needed. I have also dropped it from my v92-objexprhdlr-backport branch that I use for testing.