Fixes exception call stack for C++ exceptions #582

Merged
merged 8 commits into from Nov 16, 2016

Projects

None yet

6 participants

@jimhester
Contributor
jimhester commented Nov 11, 2016 edited

Also adds support for populating the cppstack for Rcpp::exceptions when
using clang and a str method to make printing a little nicer.

library(Rcpp)
cppFunction('double takeLog(double val) {
    if (val <= 0.0) {
      throw std::range_error("Inadmissible value");
    }
    return log(val);
  }')

# Regular exceptions now work as before
takeLog(-1)
#> Error in takeLog(-1): Inadmissible value
tryCatch(takeLog(-1), error = identity)
#> <std::range_error in takeLog(-1): Inadmissible value>
str(tryCatch(takeLog(-1), error = identity))
#> List of 3
#>  $ message : chr "Inadmissible value"
#>  $ call    : language takeLog(-1)
#>  $ cppstack: NULL
#>  - attr(*, "class")= chr [1:4] "std::range_error" "C++Error" "error" "condition"

# Rcpp exceptions now work with clang as well, and the
# str method prints the full cpp stacktrace
cppFunction('double takeLog(double val) {
    if (val <= 0.0) {
      throw Rcpp::exception("Inadmissible value");
    }
    return log(val);
  }')
str(tryCatch(takeLog(-1), error = identity))
#> List of 3
#>  $ message : chr "Inadmissible value"
#>  $ call    : language takeLog(-1)
#>  $ cppstack:
#>  stack_trace(char const*, int)
#>    Rcpp::exception::exception(char const*)
#>    Rcpp::exception::exception(char const*)
#>    takeLog(double)
#>    sourceCpp_7_takeLog
#>    do_dotcall
#>    Rf_eval
#>    Rf_applyClosure
#>    Rf_eval
# ... manually removed for brevity
#>  - attr(*, "class")= chr [1:4] "Rcpp::exception" "C++Error" "error" "condition"

Fixes #579

@eddelbuettel
Member

Looks very promising. I had been pondering releasing this weekend but may as well push back a week.

R/exceptions.R
@@ -33,4 +33,6 @@
warnings
}
-
+`str.Rcpp_stack_trace` <- function(x, ...) {
@eddelbuettel
eddelbuettel Nov 11, 2016 edited Member

Not a fan of the ` style for functions, personally. Adds little here but oh well -- we have a mixed bag of styles in the code already.

@jimhester
jimhester Nov 11, 2016 Contributor

This is actually a holdover from an earlier iteration when the method was for Rcpp::exception, which made the backticks (or quotes) necessary. I can remove them now.

@eddelbuettel
eddelbuettel Nov 11, 2016 Member

Almost not worth a commit; we can clean up another time. We still have inconsistent white-space and whatnot.

Oh, and there he did :) Thanks.

inst/include/Rcpp/exceptions.h
+ SEXP sys_calls_symbol = Rf_install( "sys.call" ) ;
+
+ // -9 Skips the wrapped tryCatch from Rcpp_eval
+ Rcpp::Shield<SEXP> sys_calls_expr( Rf_lang2(sys_calls_symbol, Rf_ScalarInteger(-9)) );
@kevinushey
kevinushey Nov 11, 2016 Contributor

I wonder if we can do something a bit more robust here? (Does this work in the presence of eager byte-compilation?)

@eddelbuettel
eddelbuettel Nov 12, 2016 Member

Any thoughts on how?

@jjallaire
jjallaire Nov 13, 2016 Member

At the last R Summit Luke warned against hard coding the number of calls on the stack so I think this might indeed fail in the presence of said eager byte-compilation.

@jjallaire
jjallaire Nov 13, 2016 Member

I guess you could loop up the stack looking for the name of the target enclosing function?

@jimhester
jimhester Nov 13, 2016 Contributor

Yes the other way to do it would be to go back to using sys.calls() and return the call just prior to tryCatch(sys.calls(), error = identity, interrupt = identity), which is what we call from the Rcpp_eval at https://github.com/RcppCore/Rcpp/pull/582/files#diff-5b6f3a71a6058dccfe3c3426c64110d2L132.

For example making a small change [1] to this PR, running the tryCatch(takeLog-1), error = identity) example from above, setting a breakpoint at exception.h:135 and running Rf_PrintValue(calls) gets you this callstack [2].

Writing the code to properly and robustly match [[6]] below is not something I have quite worked out how to do yet, which is why I went for the simpler solution to this PR initially, suggestions welcome.

[1]

diff --git a/inst/include/Rcpp/exceptions.h b/inst/include/Rcpp/exceptions.h
index b35d4bb..cd8e46f 100644
--- a/inst/include/Rcpp/exceptions.h
+++ b/inst/include/Rcpp/exceptions.h
@@ -127,10 +127,10 @@ namespace Rcpp{
 } // namespace Rcpp

 inline SEXP get_last_call(){
-    SEXP sys_calls_symbol = Rf_install( "sys.call" ) ;
+    SEXP sys_calls_symbol = Rf_install( "sys.calls" ) ;

     // -9 Skips the wrapped tryCatch from Rcpp_eval
-    Rcpp::Shield<SEXP> sys_calls_expr( Rf_lang2(sys_calls_symbol, Rf_ScalarInteger(-9)) );
+    Rcpp::Shield<SEXP> sys_calls_expr( Rf_lang1(sys_calls_symbol) );
     Rcpp::Shield<SEXP> calls( Rcpp_eval( sys_calls_expr, R_GlobalEnv ) );
     return calls;
 }

[2]

[[1]]
tryCatch(takeLog(-1), error = identity)

[[2]]
tryCatchList(expr, classes, parentenv, handlers)

[[3]]
tryCatchOne(expr, names, parentenv, handlers[[1L]])

[[4]]
doTryCatch(return(expr), name, parentenv, handler)

[[5]]
takeLog(-1)

[[6]]
tryCatch(evalq(sys.calls(), <environment>), error = function (x)
x, interrupt = function (x)
x)

[[7]]
tryCatchList(expr, classes, parentenv, handlers)

[[8]]
tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]),
    names[nh], parentenv, handlers[[nh]])

[[9]]
doTryCatch(return(expr), name, parentenv, handler)

[[10]]
tryCatchList(expr, names[-nh], parentenv, handlers[-nh])

[[11]]
tryCatchOne(expr, names, parentenv, handlers[[1L]])

[[12]]
doTryCatch(return(expr), name, parentenv, handler)

[[13]]
evalq(sys.calls(), <environment>)

[[14]]
eval(substitute(expr), envir, enclos)
@hadley
hadley Nov 14, 2016 Contributor

ccing @lionel- since he's been working on related (R-level) code

@jimhester
jimhester Nov 14, 2016 edited Contributor

bb82bd9 now matches the expression exactly rather than relying on the position in the call stack.

Is it not as efficient as it could be, a couple intermediate expressions used in multiple conditions could be stored, I wrote it this way for readabilities sake.

@lionel-
lionel- Nov 14, 2016 Contributor

The tricky part of looking at sys.calls() and related functions is to ignore intervening frames. rlang:::trail_make() is one way of getting the vector of relevant parent frames.

I think if you run rlang::call_stack() instead of sys.calls() here, you should get only a handful of frames instead of 14.

jimhester added some commits Nov 11, 2016
@jimhester jimhester Fixes exception call stack for C++ exceptions
Also adds support for populating the cppstack for Rcpp::exceptions when
using clang and a str method to make printing a little nicer.
6afcd4f
@jimhester jimhester Remove unnecessary backticks 1f7c623
@jimhester jimhester Match the Rcpp_eval tryCatch(eval(sys.call(), ...)) call explicitly bb82bd9
@jjallaire
Member

We can't take a dependency on rlang (we don't depend on any packages since 2/3 of CRAN recursively depends on us) so if we wanted to use that approach we'd need to copy the implementation into Rcpp (but it sounds like we may already have a solution that works well enough?)

@jimhester
Contributor

@jjallaire Yes there's definitely no need for an additional dependency here.

@lionel-
Contributor
lionel- commented Nov 14, 2016

yup I was only suggesting to copy trail_make()'s logic in case it made the task easier.

inst/include/Rcpp/exceptions.h
+ // We want the call just prior to the call from Rcpp_eval
+ // This conditional matches
+ // tryCatch(evalq(sys.calls(), .GlobalEnv), error = identity, interrupt = identity)
+ if (TYPEOF(expr) == LANGSXP &&
@kevinushey
kevinushey Nov 14, 2016 Contributor

Can we refactor this into a separate function (e.g. in the internal namespace just above here)? E.g. internal::is_Rcpp_eval_call or something like that.

@jimhester
jimhester Nov 14, 2016 Contributor

Done in 9412f1f

inst/include/Rcpp/exceptions.h
+ Rf_length(expr) == 4 &&
+ nth(expr, 0) == tryCatch_symbol &&
+ CAR(nth(expr, 1)) == evalq_symbol &&
+ CAR(nth(nth(expr, 1), 1)) == sys_calls_symbol &&
@kevinushey
kevinushey Nov 14, 2016 Contributor

Do we need to validate that the evalq call has a size of 3 here?

@jimhester
jimhester Nov 14, 2016 Contributor

We don't need to, see #582 (comment), but we can do so if you think it is better to be explicit about it.

@kevinushey
kevinushey Nov 14, 2016 Contributor

Ah, I wasn't aware that nth validated the length already; thanks!

@kevinushey
Contributor
kevinushey commented Nov 14, 2016 edited

LGTM barring some nitpicks -- it's probably an issue we'll never encounter in practice but it seems like the current iteration of the code could fail if it found a stack frame like

tryCatch(evalq(1 + 1), error = identity, interrupt = identity)

Ie, a somewhat more 'bare' evalq call inside of a tryCatch block.

@jimhester
Contributor

This is robust to the list size throughout, nth() checks the list length and returns a R_NilValue if the requested item is larger than the length. CAR(R_NilValue) == R_NilValue and CDR(R_NilValue) == R_NilValue, so all of the conditions will just return false in that case.

The function actually works perfectly fine if you remove the first two conditions entirely.

TYPEOF(expr) == LANGSXP &&
          Rf_length(expr) == 4

I had them in there more for explicitness than anything else.

@kevinushey
Contributor

I wasn't aware that nth() did a length check already -- thanks for pointing that out! LGTM.

@jimhester
Contributor

Also I am happy to add some tests for this functionality to prevent future regressions, is there an existing test file that would be appropriate to put them in or should I make new ones?

@eddelbuettel
Member

is there an existing test file that would be appropriate to put them in or should I make new ones?

Not sure. It is a little all over the place in inst/unitTests/ and inst/unitTests/cpp. Just apply some good judgment as usual. A new file (or pair) for exceptions may be cleanest.

@eddelbuettel
Member

Apparently we already had is_Rcpp_eval_call() somewhere else:
https://travis-ci.org/RcppCore/Rcpp/builds/175788590

@jimhester Can you take another look?

@eddelbuettel
Member

Silly me. Standard case of multiple compilation units sharing code from a header. I guess making the function inline would do it?

inst/include/Rcpp/exceptions.h
+ // We want the call just prior to the call from Rcpp_eval
+ // This conditional matches
+ // tryCatch(evalq(sys.calls(), .GlobalEnv), error = identity, interrupt = identity)
+ bool is_Rcpp_eval_call(SEXP expr) {
@eddelbuettel
eddelbuettel Nov 14, 2016 Member

Please try inline bool

@jimhester
Contributor

@eddelbuettel Yep, just did so in 65e502d, will write some tests in a few.

inst/include/Rcpp/exceptions.h
@@ -126,40 +126,49 @@ namespace Rcpp{
} // namespace Rcpp
-inline SEXP nth(SEXP s, int n) {
- return Rf_length(s) > n ? (n == 0 ? CAR(s) : CAR(Rf_nthcdr(s, n))) : R_NilValue;
+namespace internal {
@eddelbuettel
eddelbuettel Nov 14, 2016 Member

Looks like this accidentally was left outside of the outer namespace Rcpp bombing compilation:

https://travis-ci.org/RcppCore/Rcpp#L2618

na.cpp:27:12: error: reference to β€˜internal’ is ambiguous
     return internal::Rcpp_IsNA(x);
            ^
In file included from /home/travis/build/RcppCore/Rcpp/Rcpp.Rcheck/Rcpp/include/RcppCommon.h:122:0,
                 from /home/travis/build/RcppCore/Rcpp/Rcpp.Rcheck/Rcpp/include/Rcpp.h:27,
                 from na.cpp:22:
/home/travis/build/RcppCore/Rcpp/Rcpp.Rcheck/Rcpp/include/Rcpp/exceptions.h:129:20: note: candidates are: namespace internal { }
 namespace internal {
                    ^
In file included from /home/travis/build/RcppCore/Rcpp/Rcpp.Rcheck/Rcpp/include/Rcpp.h:27:0,
                 from na.cpp:22:
/home/travis/build/RcppCore/Rcpp/Rcpp.Rcheck/Rcpp/include/RcppCommon.h:45:24: note:                 namespace Rcpp::internal { }
     namespace internal {
                        ^
@jimhester
Contributor

Ok now have some simple tests for this as well.

@eddelbuettel
Member
eddelbuettel commented Nov 15, 2016 edited

This appears ready. I ran full reverse-depends checks overnight, after having run another set yesterday against master. No new issues.

I plan to merge later today. All ok?

@eddelbuettel eddelbuettel merged commit 6e0b10c into RcppCore:master Nov 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment