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

Fix unwind tests on win-builder #801

Merged
merged 3 commits into from
Jan 14, 2018
Merged

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jan 14, 2018

No description provided.

@eddelbuettel
Copy link
Member

Ok -- I had turned this into a 0.14.7.1 and it just passed -- hooray!

https://win-builder.r-project.org/251187D9Ozo1/00check.log (R-release)
https://win-builder.r-project.org/fnQpSAWO01Bo/00check.log (R-devel)

I'll just stay away from R Hub then...

@eddelbuettel eddelbuettel merged commit 5447f03 into RcppCore:master Jan 14, 2018
@eddelbuettel
Copy link
Member

BTW what I had in misc.R (would now be in stack.R) was

modified   inst/unitTests/runit.misc.R
@@ -18,6 +18,7 @@
 # along with Rcpp.  If not, see <http://www.gnu.org/licenses/>.
 
 .runThisTest <- Sys.getenv("RunAllRcppTests") == "yes"
+.onLinux <- .Platform$OS.type == "unix" && unname(Sys.info()["sysname"]) == "Linux"
 
 if (.runThisTest) {

and then use !.onLinux instead of always FALSE. Better?

@lionel-
Copy link
Contributor Author

lionel- commented Jan 14, 2018

This sounds reasonable. I completely disabled it because I don't understand the win-builder failure but on the other hand it does seem to consistently work on Linux.

@eddelbuettel
Copy link
Member

Oh for fuck's sake. I just cleaned it all up and called it 0.12.14.8, and 'check time: 0' means it died AGAIN. Just like on r-hub.

Dear package maintainer,
 
this notification has been generated automatically.
Your package Rcpp_0.12.14.8.tar.gz has been built (if working) and checked for Windows.
Please check the log files and (if working) the binary package at:
https://win-builder.r-project.org/IyUkbMAma132
The files will be removed after roughly 72 hours.
Installation time in seconds: 79
Check time in seconds: 0
R version 3.4.3 (2017-11-30)
 
All the best,
Uwe Ligges
(CRAN maintainer of binary packages for Windows)

Over to, @lionel- . That is the pristine master branch.

@eddelbuettel
Copy link
Member

And of course the same on r-devel:

Dear package maintainer,

this notification has been generated automatically.
Your package Rcpp_0.12.14.8.tar.gz has been built (if working) and checked for Windows.
Please check the log files and (if working) the binary package at:
https://win-builder.r-project.org/28GDGlEv55Vp
The files will be removed after roughly 72 hours.
Installation time in seconds: 89
Check time in seconds: 0
R Under development (unstable) (2018-01-13 r74113)

All the best,
Uwe Ligges
(CRAN maintainer of binary packages for Windows)

@eddelbuettel
Copy link
Member

I think I will switch my hobby to herding goats or something as this is beyond comprehension. 0.12.14.7.1 passed, but 0.12.14.8 does not, yet there is just about nothing material in the diff:

Binary files Rcpp-0.12.14.7.1/build/Rcpp.pdf and Rcpp-0.12.14.8/build/Rcpp.pdf differ
diff -ru Rcpp-0.12.14.7.1/ChangeLog Rcpp-0.12.14.8/ChangeLog
--- Rcpp-0.12.14.7.1/ChangeLog	2018-01-14 15:35:29.000000000 -0600
+++ Rcpp-0.12.14.8/ChangeLog	2018-01-14 17:05:14.000000000 -0600
@@ -1,8 +1,16 @@
 2018-01-14  Dirk Eddelbuettel  <edd@debian.org>
 
+        * DESCRIPTION (Version, Date): New minor version 0.12.14.8
+
         * inst/include/Rcpp/traits/is_na.h (Rcpp): Also speed up
         Rcpp::is_na<Rcomplex>
 
+2018-01-14  Lionel Henry  <lionel@rstudio.com>
+
+        * inst/unitTests/cpp/stack.cpp: Move stack unwinding tests from misc.cpp
+        * inst/unitTests/runit.stack.R: Move stack unwinding tests from misc.R;
+        disaggregate stack unwinding tests; also disable for now
+
 2018-01-11  Kirill Müller  <krlmlr@mailbox.org>
 
         * inst/include/Rcpp/traits/is_na.h: Speed up Rcpp::is_na<double>()
diff -ru Rcpp-0.12.14.7.1/DESCRIPTION Rcpp-0.12.14.8/DESCRIPTION
--- Rcpp-0.12.14.7.1/DESCRIPTION	2018-01-14 15:46:03.000000000 -0600
+++ Rcpp-0.12.14.8/DESCRIPTION	2018-01-14 17:34:01.000000000 -0600
@@ -1,6 +1,6 @@
 Package: Rcpp
 Title: Seamless R and C++ Integration
-Version: 0.12.14.7.1
+Version: 0.12.14.8
 Date: 2018-01-14
 Author: Dirk Eddelbuettel, Romain Francois, JJ Allaire, Kevin Ushey, Qiang Kou,
  Nathan Russell, Douglas Bates and John Chambers
@@ -24,4 +24,4 @@
 MailingList: Please send questions and comments regarding Rcpp to rcpp-devel@lists.r-forge.r-project.org
 RoxygenNote: 6.0.1
 NeedsCompilation: yes
-Packaged: 2018-01-14 21:46:03.86255 UTC; edd
+Packaged: 2018-01-14 23:34:01.211863 UTC; edd
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-attributes.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-attributes.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-extending.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-extending.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-FAQ.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-FAQ.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-introduction.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-introduction.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-jss-2011.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-jss-2011.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-modules.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-modules.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-package.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-package.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-quickref.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-quickref.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-sugar.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-sugar.pdf differ
Binary files Rcpp-0.12.14.7.1/inst/doc/Rcpp-unitTests.pdf and Rcpp-0.12.14.8/inst/doc/Rcpp-unitTests.pdf differ
diff -ru Rcpp-0.12.14.7.1/inst/NEWS.Rd Rcpp-0.12.14.8/inst/NEWS.Rd
--- Rcpp-0.12.14.7.1/inst/NEWS.Rd	2018-01-14 15:35:29.000000000 -0600
+++ Rcpp-0.12.14.8/inst/NEWS.Rd	2018-01-14 17:06:19.000000000 -0600
@@ -11,6 +11,9 @@
       set an initial format string (Dirk in \ghpr{777} fixing \ghit{776}).
       \item The 'new' Date and Datetime vectors now have \code{is_na} methods
       too. (Dirk in \ghpr{783} fixing \ghit{781}).
+      \item Protect more temporary \code{SEXP} objects produced by \code{wrap}
+      (Kevin in \ghpr{784}).
+      \item Use public R APIs for \code{new_env} (Kevin in \ghpr{785}).
       \item Evaluation of R code is now safer when compiled against R
       3.5 (you also need to explicitly define \code{RCPP_PROTECTED_EVAL}
       before including \code{Rcpp.h}). Longjumps of all kinds (condition
@@ -36,6 +39,10 @@
       \item Permit compilation on ANDROID (Kenny Bell in \ghpr{796}).
       \item Improve support for NVCC, the CUDA compiler (Iñaki Ucar in
       \ghpr{798} addressing \ghit{797}).
+      \item Speed up tests for NA and NaN (Kirill and Dirk in \ghpr{799} and
+      \ghpr{800}).
+      \item Rearrange stack unwind test code, keep test disabled for now (Lionel
+      in \ghpr{801}).
     }
     \item Changes in Rcpp Attributes:
     \itemize{

@lionel-
Copy link
Contributor Author

lionel- commented Jan 15, 2018

Sounds like it's undeterministic :/

Did this also happen with a version anterior to the protection stuff?

@eddelbuettel
Copy link
Member

I am so bloody confused. I am re-submitting 0.12.14.7.1 as 0.12.14.9 just to check. It passed as the former, I just want to rule out that the 5 tokens matterd. "Usually" we do a.b.c for cran, and that does fewer checks, and a,b,c.d for tests. The comparison should just be > 3 and not have an impact.

But I am generally confused now.

@lionel- lionel- deleted the fix-unwind branch January 15, 2018 00:52
@eddelbuettel
Copy link
Member

Does anybody have an idea how to repair this to use the code? @lionel- @krlmlr @kevinushey ?

At best as I can tell, we seem to be in a semi-random situation where the code sometimes fails on Windows, and sometimes it doesn't.

My inclination to undo #789 / condition it away. Better suggestions?

@eddelbuettel
Copy link
Member

So the tarball of the current master

  • failed win-builder yesterday
  • passes today

WTF?

@kevinushey
Copy link
Contributor

I would vote that we take out the protected evaluation stuff for now altogether.

FWIW when running the Rcpp unit tests within RStudio on my macOS machine, I see that 'sometimes' things blow up (RStudio gets into an infinite loop printing error messages) so there may well be something non-deterministic that's causing errors here. (Maybe something that should be getting protected from the garbage collector isn't?)

Since @lionel- is on vacation I'll try posting to R-devel to see if they have any comments.

@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 15, 2018

I was thinking that too. Ie post-process Lionel's patch with a few #ifdef ?

@krlmlr
Copy link
Contributor

krlmlr commented Jan 15, 2018

So far I have 3 out of 5 successful builds of b52f690 on WinBuilder.

@kevinushey
Copy link
Contributor

kevinushey commented Jan 15, 2018

I've contacted Luke Tierney to ask for some more guidance. FWIW I saw this ASAN error locally today with my longjmp package:

memory.c:3747:5: runtime error: member access within null pointer of
type 'struct SEXPREC'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior memory.c:3747:5 in
memory.c:3747:5: runtime error: member access within null pointer of
type 'struct sxpinfo_struct'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior memory.c:3747:5 in
ASAN:DEADLYSIGNAL
=================================================================
==93083==ERROR: AddressSanitizer: SEGV on unknown address
0x00011cfda220 (pc 0x000101480b59 bp 0x7ffeefbfae90 sp 0x7ffeefbfae60
T0)
==93083==The signal is caused by a READ memory access.
    #0 0x101480b58 in SETCAR memory.c:3747
    #1 0x101278714 in R_UnwindProtect context.c:904
    #2 0x11e5d5bc7 in longjmp longjmp.cpp:27
    #3 0x1012b3039 in R_doDotCall dotcode.c:567
    #4 0x10130013d in do_dotcall dotcode.c:1252
    #5 0x1013aeccd in Rf_eval eval.c:727
    #6 0x1013f6679 in do_begin eval.c:2362
    #7 0x1013ae97d in Rf_eval eval.c:699
    #8 0x1013f1f04 in R_execClosure eval.c
    #9 0x1013aee38 in Rf_eval eval.c:747
    #10 0x101461118 in Rf_ReplIteration main.c:258
    #11 0x101465160 in R_ReplConsole main.c:308
    #12 0x101464f37 in run_Rmainloop main.c:1082

so I suspect there may be something going on (a missed PROTECT?) in the R-devel sources. A missed protect would explain the non-determinism we're seeing in testing Rcpp here.

@kevinushey
Copy link
Contributor

kevinushey commented Jan 15, 2018

@eddelbuettel: right; I think we should just guard the protected eval with

#if false // disabled until dust around R_UnwindProtect has settled

or something similar.

@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 15, 2018

I think I add an #ifdef on our side for something like RCPP_USE_UNWIND_PROTECT and let it default to FALSE.

Note that I am pretty certain that I also saw crappy outcomes with R-release. How about your experiments on macOS? Trouble with both, or only R-devel?

@krlmlr
Copy link
Contributor

krlmlr commented Jan 15, 2018

All 5 builds good. Was I testing the wrong version?

@eddelbuettel
Copy link
Member

No, you just have better karma than us.

Who knows maybe it also depends on the state of the machine. Did you send five to both R versions, or just one each?

@krlmlr
Copy link
Contributor

krlmlr commented Jan 15, 2018

To both.

@eddelbuettel
Copy link
Member

And you got ten winners? Astounding ...

@krlmlr
Copy link
Contributor

krlmlr commented Jan 16, 2018

With check times of ~1500s, though. That's different from CRAN's check times of 500s max, but might be a different machine too.

@eddelbuettel
Copy link
Member

Something is going over there because I got several reports about a top-level file .idea as well as CMake files. I can assure you I don't have either.

@krlmlr
Copy link
Contributor

krlmlr commented Jan 16, 2018

I saw these too, but then it seems fairly unlikely that these have any effect (except for the longer check times, of course). But who knows?

Are you getting e-mails from WinBuilder even when I change the e-mail address when submitting?

@eddelbuettel
Copy link
Member

No, and I am pretty sure I got mine (as I ended up with new/unused minor versions).

But "they" did some rejigging over the break anyway (or tried to) so who knows.

@lionel-
Copy link
Contributor Author

lionel- commented Jan 16, 2018

@kevinushey About the ASAN warning, it looks like one of the SEXP in the SETCAR in R_ProtectUnwind might be uninitialised. Since that can't be the token, I guess it's R_ReturnedValue here. Also ASAN notes "The signal is caused by a READ memory access." which corroborates that the token is not at fault. If this hypothesis is right, we shouldn't be too concerned with this warning, it may be because R_ReturnedValue is a global variable and ASAN doesn't know it can't be a C NULL once R has launched.

@krlmlr Maybe the double time is because on r-devel the examples and tests are run on both i386 and x64 arch. On r-release only the examples are run on both arch.

@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 16, 2018

@lionel- No, --as-cran now summarises time for each arch, and we usually get ~500 secs each. Something was wrong at their end.

@kevinushey
Copy link
Contributor

kevinushey commented Jan 16, 2018

@lionel- the ASAN warning is not just a warning -- it leads to a (intermittent) crash / infinite loop / unpredictable behavior when run in R without sanitizers available. The API also does not work on Windows with 32bit versions of R.

@lionel-
Copy link
Contributor Author

lionel- commented Jan 16, 2018

Got it, I'm not familiar with this ASAN/UBSAN thing yet. Since it's about one of the SEXP being a C NULL (if I understand the error correctly), I don't think this is about a protection issue though.

@eddelbuettel
Copy link
Member

I recently watched a truly awesome talk on the sanitizers -- including the newer ones on memory races and what not. But I can't find it right now :-/ Likely from cppcon, maybe from 2016.

The various *SAN are the closest we have to a free lunch in C++ land. CRAN is right in pushing their use.

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

Successfully merging this pull request may close these issues.

None yet

4 participants