Clean up (or remove) protect-unwind API and Rcpp_fast_eval

[eddelbuettel]

So #789 by @lionel- turned out to have had side effects. It is currently in hiding and #ifdef-ed away.

We could try to clean it up for the next release in March, or maybe the one thereafter in May (if we keep the same cadence). If we don't then we should take it out at some point. No rush, but should not get forgotten either.

[kevinushey]

The RStudio team is going to be getting together in San Diego next week after rstudio::conf so hopefully @lionel- and I will have time to sit down and take a closer look.

The fact that the unwind stuff uniformly crashes on Windows is not promising, though (even with Rcpp out of the picture)

[kevinushey]

Good news from Luke Tierney: he was able to reproduce the unwind crash I had seen on macOS / Linux and has a fix in R-devel for that.

If we reintroduce the usage of the unwind API we might still have to keep it turned off on Windows, though. (I think the crash may be toolchain related, but need to investigate a little more)

[eddelbuettel]

@lionel- Rcpp 0.12.16 is out, so we would have some time to do damage if you want to revisit this now. Later is fine too.

[lionel-]

Thanks I'll revisit within a few weeks.

[lionel-]

Rcpp::internal::LongjumpException should not be internal to allow other packages to call potentially-jumping functions of the R API with protect-unwind. If the call fails, they should be allowed to throw a LongjumpException with their own unwind token.

[eddelbuettel]

Looks like it was you who put it there, so there are no other users yet. In that sense we could make that change.

But wasn't there some other verboten, don't do that state around longjumps? Or is my memory foggy?

[lionel-]

wasn't there some other verboten, don't do that state around longjumps

Are you referring to catching a LongjumpException? For protect-unwind it should be fine to continue a longjump with a token from someone else as long as the token was validly generated.

So LongjumpException shouldn't be caught by a third party without being rethrown (otherwise the longjump is interrupted and the token memory is leaked), but it's fine for a third party to throw their own LongjumpException. This also means LongjumpException has to be public so it can be rethrown before a catch (...).

Oh hmm maybe we need to think through what happens when another token is rethrown from a destructor (that could happen for instance if Rcpp_fast_eval() is called from a dtor). That would mean we leak the original token.

Edit: Ah we can release the token from the exception dtor. However it seems we can't guarantee proper propagation of an R longjump from a dtor because there might be another thrown exception on the stack. With C++11 it seems we might be able to get a hold on the longjump exception currently being thrown (with current_exception()) and update it with a new token. Still it seems that evaluating R code from a dtor should generally be avoided.

[eddelbuettel]

Sounds like something where we'd probably want to pass a spec or design to Luke for a glance at it.

[eddelbuettel]

@lionel- Gentle poke. It is not urgent -- I'll probably prepare the next Rcpp release this week but if this is something we want to come back to for the next cycle let me know.

[lionel-]

Thanks, I'm planning to work on this the week after eRum.

[eddelbuettel]

Done in #859 -- with a big thank you!

[lionel-]

No worries! I hope all issues are now resolved... Let me know if anything comes up.

[eddelbuettel]

We just got a new one -- both for me and for @coatless on macOS -- the 'embedded' code submitted by Filip. Not sure it is related, and don't mean to imply that, but it is something new. :-/

[lionel-]

Yes this is unrelated to the new eval stuff. You can solve this by sourcing in a child of the base env in https://github.com/RcppCore/Rcpp/blob/master/inst/unitTests/runit.embeddedR.R#L30.

newEnv <- new.env(parent = baseenv())
...
newEnv2 <- new.env(parent = baseenv())

However I am not sure why there's a dangling foo binding in the global env.

What's weird is that it doesn't always fail.

[eddelbuettel]

Shall we open a new ticket for this? We all (ie Rcpp Core, @filipsch , ...) would probably benefit from what you know about environments and scoping...

[lionel-]

However I am not sure why there's a dangling foo binding in the global env.

Hmm it could also be in test.embeddedR() exec env or the RUnit sandbox for this file. Global env is a prime suspect because that's the default for sourceCpp().

Shall we open a new ticket for this? We all (ie Rcpp Core, @filipsch , ...) would probably benefit from what you know about environments and scoping...

Sure though I'm not sure when I'll have time to investigate this.

[eddelbuettel]

That's ok, we are all busy as hell. @filipsch is the one who needs it and is most likely to drive this. If you can just help him by answering questions on ideas, code, design, approach,... it would help.

Else I may be forced to revert this as I am unlikely to release 0.12.18 with a known failure.

[lionel-]

By "this" you mean #852 right? It's unlikely to have introduced a new problem, the issue must be already there (perhaps in Rcpp but more likely in the unit tests).

[eddelbuettel]

Yes, what I turned off for now in this commit 5d5cdb8.