turning off Eigen debug

[Bob Carpenter]

Turning off debug isn't as straightforward as
I'd hoped. There are two options.

Option 1: put flags in the makefile that make
it easy to turn off all range checking in
Stan and in Eigen.

Pros: gets rid of computation
Cons: will seg fault instead of report errors
won't help with models in Stan language

If that's all people want, no problem.

It's hard to control through the modeling language
because it would require recompiling the library
libstan.a with different compiler flags.


Option 2: we could more subtly try to control
the range checks in the modeling language and Eigen.

First, do no harm. Wherever possible, we want to avoid
the possibility of seg faults percolating through to
R and crashing it.

The current code is redundant for indexing.
Stan checks std::vector, Eigen vector, row vector
and matrix indexings generated from the modeling language.
Eigen also checks all Eigen vector, row vector and
matrix indexing and raises an indexing exception if
it finds one rather than seg faulting.

If Eigen's literally just doing assert() in the code,
I don't know that that's much better, and we probably
need to add a bunch of range checks on Eigen operations
to be safe.

Nothing in Stan checks that matrices are compatible
sizes before multiplying. I'm not sure if Eigen does
that in the same way it checks indexing.

The doc for EIGEN_NO_DEBUG says it turns
off all asserts at both compile and run time:

http://eigen.tuxfamily.org/dox/TopicPreprocessorDirectives.html

Controlling our own checks one way or the other is no
problem.

- Bob

[Ben Goodrich]

On Tuesday, September 4, 2012 6:18:36 PM UTC-4, Bob Carpenter wrote:

Option 2:  we could more subtly try to control
the range checks in the modeling language and Eigen.

First, do no harm.  Wherever possible, we want to avoid
the possibility of seg faults percolating through to
R and crashing it.

Yes, although I think that is an RStan issue more than a Stan issue. In other words, I don't think it is a huge problem if an executable that is called from the command line segfaults. As I said before, I think it is possible for RStan to catch any SIGABRT and call exit() to return gracefully to R.
 

Nothing in Stan checks that matrices are compatible
sizes before multiplying.  I'm not sure if Eigen does
that in the same way it checks indexing.

It can be a compiler error for the fixed size matrices, but we don't use those except for the one-column matrices. For a MatrixXd, attempting to multiply incompatible matrices seems to result in a segfault regardless of whether NDEBUG is defined.

Ben

[Bob Carpenter]

Thanks for the clarifications. Comments below.

I take it that's OK then for you (Ben) if we supply
an unsafe compile control (like -NDEBUG) that turns all
range and dimension checking off?

More below on the R interface.

On 9/4/12 7:01 PM, Ben Goodrich wrote:
> On Tuesday, September 4, 2012 6:18:36 PM UTC-4, Bob Carpenter wrote:
>
> Option 2: we could more subtly try to control
> the range checks in the modeling language and Eigen.
>
> First, do no harm. Wherever possible, we want to avoid
> the possibility of seg faults percolating through to
> R and crashing it.
>
>
> Yes, although I think that is an RStan issue more than a Stan issue. In other words, I don't think it is a huge problem
> if an executable that is called from the command line segfaults. As I said before, I think it is possible for RStan to
> catch any SIGABRT and call exit() to return gracefully to R.

If I understood correctly, Jiqiang was reporting today
that whatever you suggested didn't work and things
still crashed in R.

I didn't quite understand what he was doing, but I
think it was because he was letting the Rcpp macros
do the try/catch, which apparently just get thrown to
R (I don't know if they're type converted).

I suggested just doing our own try/catch inside the scope
of whatever Rcpp wants and then handling the exceptions
in our code.

try {
call Stan...
} catch (std::exception& e) {
report using e.what() ...
DO NOT RETHROW
}

The ref in the catch is important because it'll slice
otherwise.

> Nothing in Stan checks that matrices are compatible
> sizes before multiplying. I'm not sure if Eigen does
> that in the same way it checks indexing.
>
>
> It can be a compiler error for the fixed size matrices, but we don't use those except for the one-column matrices. For a
> MatrixXd, attempting to multiply incompatible matrices seems to result in a segfault regardless of whether NDEBUG is
> defined.

Argh.

So we need to check that anyway to be safe.
That should be run through our matrix lib.

I just added another to-do item for it.

- Bob

[Ben Goodrich]

On Tuesday, September 4, 2012 7:19:18 PM UTC-4, Bob Carpenter wrote:

Thanks for the clarifications.  Comments below.

I actually unclarified the issue (I accidentally tested with an uninitialized matrix before).

FIXED-SIZE: Compiler error
DYNAMIC-SIZE: Run-time assert if -NDEBUG is not defined
DYNAMIC-SIZE: Bad news if -NDEBUG is defined

Worse, the attached program asserts by default but produces the wrong answer if compiled with -DNDEBUG on my machine. If it is the same with you, I think it is a bug in Eigen because this page seems to imply it should segfault.

http://eigen.tuxfamily.org/dox/TutorialMatrixArithmetic.html#TutorialArithmeticValidity

I take it that's OK then for you (Ben) if we supply
an unsafe compile control (like -NDEBUG) that turns all
range and dimension checking off?

Fine with me.

Ben

[Ben Goodrich]

On Tuesday, September 4, 2012 7:19:18 PM UTC-4, Bob Carpenter wrote:

I suggested just doing our own try/catch inside the scope
of whatever Rcpp wants and then handling the exceptions
in our code.

try {
   call Stan...
} catch (std::exception& e) {
   report using e.what() ...
   DO NOT RETHROW
}

The ref in the catch is important because it'll slice
otherwise.

For a SIGABRT, it may be insufficient to catch an exception because

https://en.wikipedia.org/wiki/SIGABRT

says that SIGABRT signal can't be blocked. I think you have to exit() before the signal gets to R.
 
Ben

[Jiqiang Guo]

On Tue, Sep 4, 2012 at 7:38 PM, Ben Goodrich <goodri...@gmail.com> wrote:

On Tuesday, September 4, 2012 7:19:18 PM UTC-4, Bob Carpenter wrote:

I suggested just doing our own try/catch inside the scope
of whatever Rcpp wants and then handling the exceptions
in our code.

try {
   call Stan...
} catch (std::exception& e) {
   report using e.what() ...
   DO NOT RETHROW
}

In one example, I changed the try/cacth to the above suggested (not using the one 

defined by Rcpp).  But I still cannot see the error message. 

 

The ref in the catch is important because it'll slice
otherwise.

For a SIGABRT, it may be insufficient to catch an exception because

https://en.wikipedia.org/wiki/SIGABRT

says that SIGABRT signal can't be blocked. I think you have to exit() before the signal gets to R.
 

I do not know how to exit before the signal gets to R.  My understanding it that in rstan, stan is running with the same process of R. So once a SIGABRT is triggered, the function, if any, defined by R to deal with that signal would be called.  I cannot see what I can do about signal here.  

 

Ben

--
 
 

[Ben Goodrich]

On Tuesday, September 4, 2012 9:09:49 PM UTC-4, Jiqiang Guo wrote:

I do not know how to exit before the signal gets to R.  My understanding it that in rstan, stan is running with the same process of R. So once a SIGABRT is triggered, the function, if any, defined by R to deal with that signal would be called.  I cannot see what I can do about signal here.  

I see now. Calling exit() actually exists from R. Should RStan (at some point in the future) spawn a separate thread? I would guess this is going to be a headache on Windows.

Ben

[Bob Carpenter]

I get what SIGABRT does. Is that really
what we have? What's throwing it?

Maybe the model code needs to catch things
earlier.

- Bob

On 9/4/12 9:09 PM, Jiqiang Guo wrote:

> --
>
>

[Bob Carpenter]

I now definitely think we need to add tests to all
of Stan's matrix operations. With a compiler flag
to suppress all the tests for the open-wheel
racing set. More below.

On 9/4/12 7:34 PM, Ben Goodrich wrote:
> On Tuesday, September 4, 2012 7:19:18 PM UTC-4, Bob Carpenter wrote:
>
> Thanks for the clarifications. Comments below.
>
>
> I actually unclarified the issue (I accidentally tested with an uninitialized matrix before).
>
> FIXED-SIZE: Compiler error
> DYNAMIC-SIZE: Run-time assert if -NDEBUG is not defined

I retract my previous question. assert() throws
a SIGABRT, so that would explain where they come
from.

> DYNAMIC-SIZE: Bad news if -NDEBUG is defined
>
> Worse, the attached program asserts by default but produces the wrong answer if compiled with -DNDEBUG on my machine. If
> it is the same with you, I think it is a bug in Eigen because this page seems to imply it should segfault.
>
> http://eigen.tuxfamily.org/dox/TutorialMatrixArithmetic.html#TutorialArithmeticValidity

I don't see where they imply it'll segfault per se,
but they do say it'll "probably crash". I definitely
think crashing is better than returning the wrong
answer.

Well, that makes me understand why Boost
went to the trouble it did to make its error
behavior configurable! (Though there's an
ignore error, there's not a turn-off-test option,
though the errors may get compiled away if they're
no-ops.)

- Bob

[Bob Carpenter]

On 9/4/12 9:50 PM, Ben Goodrich wrote:
> On Tuesday, September 4, 2012 9:09:49 PM UTC-4, Jiqiang Guo wrote:
>
> I do not know how to exit before the signal gets to R. My understanding it that in rstan, stan is running with the
> same process of R. So once a SIGABRT is triggered, the function, if any, defined by R to deal with that signal would
> be called. I cannot see what I can do about signal here.
>
>
> I see now. Calling exit() actually exists from R. Should RStan (at some point in the future) spawn a separate thread? I

A thread won't be enough of a firewall against
a process abort.

It would be great if we could fork a process
as it would make sure we don't take R down
with Stan if Stan crashes. But then that makes
getting the data back trickier.

That doesn't mean we shouldn't protect all the ops
in Stan!

> would guess this is going to be a headache on Windows.

Maybe, unless we hide behind someone else's hard
work on an abstraction layer. Like maybe one of
those parallel process packages in R -- that might
kill two birds with one stone.

- Bob

[Ben Goodrich]

On Tuesday, September 4, 2012 11:43:56 PM UTC-4, Bob Carpenter wrote:

I get what SIGABRT does.  Is that really
what we have?  What's throwing it?

Maybe the model code needs to catch things
earlier.

I believe in the original situation was trying to assign out-of-range to a std::vector within transformed parameters {} . So, I don't know how to catch that earlier, unless you want to clutter every for loop with a

for(unsigned int i = 1; i < K; i++) {
  if(i < 1) // fail
  else if(i > vector.size() ) // fail
  vector[i] = something;
}

Ben

[Bob Carpenter]

The problem is that I never put any index
checks on the left hand side of assignments.

The left-hand-side of an assignment is treated
separately from general expression indexing
because the syntax for lvalues is limited.

So I'm afraid the answer is yes, I do want to put
those checks in. So the compiled code would
look similar to what it looks like for the
get_base1() calls in the code now.

for (unsigned int i = 1; i < K; i++) {
check_range(vector,i,"v");
v[i-1] = ...;
}

and we'd have a support function like:

template <typename T>
inline check_range(const std::vector<T>& x,
size_t i,
const char* msg) {
#if (!defined NDEBUG) || (!defined STAN_NO_DEBUG)
if (i < 1 || i > x.size())
... throw some kind of idx exception ...
#endif
}

We need to make sure check_range is included as a header
so it can get compiled with the right flags.

- Bob