Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clad::hessian doesn't like pointer dereferencing #961

Closed
guitargeek opened this issue Jun 27, 2024 · 13 comments
Closed

clad::hessian doesn't like pointer dereferencing #961

guitargeek opened this issue Jun 27, 2024 · 13 comments

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Jun 27, 2024

Arrays are used everywhere in the code generated by RooFit, so this needs to be supported also in Hessian mode.

Reporducer:

#include <Math/CladDerivator.h>

double roo_func_wrapper_0(double* params) {
   double arr[] = {3.0};
   return params[0] + *arr;
}

#pragma clad ON
clad::gradient(roo_func_wrapper_0, "params"); // gradient works fine!
clad::hessian(roo_func_wrapper_0, "params[0:1]");
// It also doesn't work with:
//clad::hessian<clad::opts::diagonal_only>(roo_func_wrapper_0, "params[0]");
#pragma clad OFF

void macro()
{
   std::vector<double> parametersVec = {1.0};

   roo_func_wrapper_0(parametersVec.data());
}

Output:

warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
note: consider using __builtin_trap() or qualifying pointer with 'volatile'
warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
note: consider using __builtin_trap() or qualifying pointer with 'volatile'
@vaithak
Copy link
Collaborator

vaithak commented Jun 27, 2024

I am unable to reproduce this with Clang. This works correctly with Clang as intended; is this an issue with Cling?
One thing that seems wrong here is that we specify the index from start to end (both included) for hessian.
So, this params[0:1] means there are two elements inside params by which we compute the hessian against, which is wrong (as params only has 1 element).
@guitargeek can you verify once if params[0] only fixes it? Essentially making sure that the last index is included.

@guitargeek
Copy link
Contributor Author

Using params[0] didn't fix it. Did you try to run the reproducer with ROOT?

@vaithak
Copy link
Collaborator

vaithak commented Jun 27, 2024

I am trying that next. I will let you know if I find something.

@vgvassilev
Copy link
Owner

I am thinking that we should put somehow cling in the pull request pre-merge checks…

@vgvassilev
Copy link
Owner

@guitargeek, which interface of cling we use to run the code through? It seems we have some noise from the null pointer checks. We can disable that with a pragma in ROOT IIRC or using an interface which does not enable that feature…

@guitargeek
Copy link
Contributor Author

I was running the code as a ROOT macro, i.e. putting the reproducer in macro.C and then running it like root macro.C.

You get the same error in any ROOT setup, for example also on lxplus with 6.32.02:

source /cvmfs/sft.cern.ch/lcg/app/releases/ROOT/6.32.02/x86_64-almalinux9.4-gcc114-opt/bin/thisroot.sh
root macro.C

@vgvassilev
Copy link
Owner

I am wondering how is this different than the regular calls where we avoid cling_runtime_internal_throwIfInvalidPointer. I see that TFormula goes via gInterpreter->Declare and your macro must be going via gInterpreter->ProcessLine or something like that...

@vaithak
Copy link
Collaborator

vaithak commented Jun 28, 2024

So, I tested this out with a simple example. This seems to be unrelated to Hessian, but fails with normal forward mode too.
I printed the source fn AST, turns out this function is added in the source AST itself.

  • Source fn:
double roo_func_wrapper_0(double* params) {
   return *params;
}
  • Cling AST:
FunctionDecl 0x555558fbc6b8 </home/vaithak/test_fail.C:4:1, line:9:1> line:4:8 used roo_func_wrapper_0 'double (double *)'
|-ParmVarDecl 0x555558fbc5c0 <col:27, col:35> col:35 used params 'double *'
`-CompoundStmt 0x555558fbc800 <col:43, line:9:1>
  `-ReturnStmt 0x555558fbc7f0 <line:8:4, col:38>
    `-ImplicitCastExpr 0x555558fbc7d8 <col:37, col:38> 'double' <LValueToRValue>
      `-UnaryOperator 0x555558fbc7c0 <col:37, col:38> 'double' lvalue prefix '*' cannot overflow
        `-CStyleCastExpr 0x555558fbc9b8 <col:38> 'double *' <BitCast>
          `-CallExpr 0x555558fbc968 <<invalid sloc>, col:38> 'void *'
            |-ImplicitCastExpr 0x555558fbc950 <<invalid sloc>> 'void *(*)(void *, void *, const void *)' <FunctionToPointerDecay>
            | `-DeclRefExpr 0x555558fbc8d8 <<invalid sloc>> 'void *(void *, void *, const void *)' lvalue Function 0x555558fbbbc0 'cling_runtime_internal_throwIfInvalidPointer' 'void *(void *, void *, const void *)'
            |-CStyleCastExpr 0x555558fbc850 <<invalid sloc>> 'void *' <IntegralToPointer>
            | `-IntegerLiteral 0x555558fbc818 <<invalid sloc>> 'unsigned long' 93824992439856
            |-CStyleCastExpr 0x555558fbc8b0 <<invalid sloc>> 'void *' <IntegralToPointer>
            | `-IntegerLiteral 0x555558fbc878 <<invalid sloc>> 'unsigned long' 93825053476776
            `-CStyleCastExpr 0x555558fbc928 <col:38> 'const void *' <NoOp>
              `-ImplicitCastExpr 0x555558fbc910 <col:38> 'const void *' <BitCast> part_of_explicit_cast
                `-ImplicitCastExpr 0x555558fbc7a8 <col:38> 'double *' <LValueToRValue> part_of_explicit_cast
                  `-DeclRefExpr 0x555558fbc788 <col:38> 'double *' lvalue ParmVar 0x555558fbc5c0 'params' 'double *'

Clad is unable to differentiate this function and hence the warning:

warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.

@vgvassilev
Copy link
Owner

Maybe we should add a custom propagator for this function in ROOT. Can you take a look?

@vgvassilev
Copy link
Owner

I am wondering how is this different than the regular calls where we avoid cling_runtime_internal_throwIfInvalidPointer. I see that TFormula goes via gInterpreter->Declare and your macro must be going via gInterpreter->ProcessLine or something like that...

The way that R__CLING_PTRCHECK which controls whether ROOT should generate cling_runtime_internal_throwIfInvalidPointer is suboptimal to say the least. Apparently there is not an easy way to disable this. It was implemented via annotations and there is no pragma on/off supported...

@vgvassilev
Copy link
Owner

I just found an odd way to test it:

root.exe -l -b
root [0] .rawInput
Using raw input
root [1] #include <Math/CladDerivator.h>
root [2] double roo_func_wrapper_0(double* params) {
root (cont'ed, cancel with .@) [3]   double arr[] = {3.0};
root (cont'ed, cancel with .@) [4]   return params[0] + *arr;
root (cont'ed, cancel with .@) [5]}
root [6] #pragma clad ON
root [7] auto grad = clad::gradient(roo_func_wrapper_0, "params"); // gradient works fine!
root [8] auto hess = clad::hessian(roo_func_wrapper_0, "params[0:1]");
root [9] #pragma clad OFF
root [10] .rawInput
Not using raw input
root [11] std::vector<double> parametersVec = {1.0};
root [12] roo_func_wrapper_0(parametersVec.data());
root [13] roo_func_wrapper_0(parametersVec.data())
(double) 4.0000000
root [14] 

@vgvassilev
Copy link
Owner

diff --git a/interpreter/cling/lib/Interpreter/ClingPragmas.cpp b/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
index f9179e3d61..a5d02bc1b5 100644
--- a/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
+++ b/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
@@ -60,6 +60,7 @@ namespace {
       kArgumentsAreLiterals,
 
       kOptimize,
+      kPtrCheck,
       kInvalidCommand,
     };
 
@@ -120,6 +121,8 @@ namespace {
         return kAddInclude;
       else if (CommandStr == "optimize")
         return kOptimize;
+      else if (CommandStr == "ptrcheck")
+        return kPtrCheck;
       return kInvalidCommand;
     }
 
@@ -209,6 +212,18 @@ namespace {
         CO.OptLevel = OptLevel;
   }
 
+  void PtrCheckCommand(const char* Str) {
+    char* ConvEnd = nullptr;
+    // FIXME: Check better for On/Off/Default and invalid input.
+    int PtrCheckVal = std::strtol(Str, &ConvEnd, 10 /*base*/);
+    auto T = const_cast<Transaction*>(m_Interp.getCurrentTransaction());
+    assert(T && "Parsing code without transaction!");
+    // The topmost Transaction drives the jitting.
+    T = T->getTopmostParent();
+    CompilationOptions& CO = T->getCompilationOpts();
+    CO.CheckPointerValidity = PtrCheckVal;
+  }
+
   public:
     ClingPragmaHandler(Interpreter& interp):
       PragmaHandler("cling"), m_Interp(interp) {}
@@ -250,6 +265,8 @@ namespace {
         return LoadCommand(PP, Tok, std::move(Literal));
         case kOptimize:
           return OptimizeCommand(Literal.c_str());
+        case kPtrCheck:
+          return PtrCheckCommand(Literal.c_str());
 
         default:
           do {

This patch allow us to do something like that in ROOT:

root.exe -l -b 
root [0] gInterpreter->Declare("#include <Math/CladDerivator.h> \n #pragma cling ptrcheck 0 \n double roo_func_wrapper_0(double* params) { double arr[] = {3.0}; return params[0] + *arr; } \n auto h = clad::hessian(roo_func_wrapper_0, \"params[0:1]\");")
(bool) true
root [1] .q

@guitargeek
Copy link
Contributor Author

Works now, thanks to the related issues being fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants