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

C++ exception getting lost on OSX R 3.4 #37

Closed
jeroen opened this issue May 31, 2017 · 22 comments
Closed

C++ exception getting lost on OSX R 3.4 #37

jeroen opened this issue May 31, 2017 · 22 comments

Comments

@jeroen
Copy link
Owner

jeroen commented May 31, 2017

This should result in a SyntaxError however it gives unknown reason with the new OSX toolchain.

ctx <- V8::v8()
ctx$eval('var foo = {bla}')
# c++ exception (unknown reason)

This might be due to the libcxx mixing. The problem has appeared in the wild in lawn pkg.

@kevinushey
Copy link

It's almost certainly a toolchain issue. I was able to fix the exception by manually hacking the standard library used by V8.

Here's my repro -- first, set up your Makevars as so:

CC=/usr/local/clang4/bin/clang
CXX=/usr/local/clang4/bin/clang++
LDFLAGS=-L/usr/local/clang4/lib

Then, from a clean R session, reinstall both Rcpp and V8 from sources:

devtools::install_github("RcppCore/Rcpp", force = TRUE)
devtools::install_github("jeroen/V8", force = TRUE)

Try running your snippet:

ctx <- V8::v8()
ctx$eval('var foo = {bla}')

you'll see

> ctx <- V8::v8()
> ctx$eval('var foo = {bla}')
Error in context_eval(join(src), private$context) : 
  c++ exception (unknown reason)

Now, try forcing V8.so to use the system standard library:

source <- "/usr/local/clang4/lib/libc++.1.dylib"
target <- "/usr/lib/libc++.1.dylib"
cmd <- paste(
  "install_name_tool",
  "-change",
  source,
  target,
  system.file("libs/V8.so", package = "V8")
)
system(cmd)

Restart R, and try your snippet again -- it works as expected.

> ctx <- V8::v8()
> ctx$eval('var foo = {bla}')
Error in context_eval(join(src), private$context) : 
  SyntaxError: Unexpected token } 

My best guess as to what's going on -- the actual v8 library is using the system's C++ standard library, emitting an exception, and that's getting handled by the V8 package which is using the R-provided C++ standard library, and some difference in ABI is causing it to not be recognized / handled as expected.

@kevinushey
Copy link

Some possible ways forward:

  1. V8 could try downloading + building the v8 libraries during R CMD INSTALL,
  2. V8 could add this install_name_tool hack as some kind of post-install step.

I'm not very optimistic that R Core would make any change to fix this issue at this point. :-/

@jeroen
Copy link
Owner Author

jeroen commented May 31, 2017

This case is not a blocker for me, most other exceptions seem to work fine for some reason. There is some special thing about the syntax exception that triggers this. But it's probably good to be aware that the toolchain mixing is not 100% waterproof.

@s-u
Copy link

s-u commented Jun 1, 2017

Yes, we are between a rock and a hard place - Apple clang is outdated and doesn't support OMP, so we're using clang4 to have a modern compiler with C++ support, but I guess even within stdc++ there are API incompatibilities wrt exceptions which is unfortunate. Just for the fun of it - did you try forcing DYLD_LIBARY_PATH to the clang4 libraries and/or re-directing V8 to clang4's stdc++? I'm just wondering if this is an issue in code generation (i.e. they are truly incompatible) or runtime (in which case using the more recent one could work).

@kevinushey
Copy link

Ah, it looks like this version of libv8 (3.15) builds using the older standard library by default:

kevin@cdrv:/usr/local/opt/v8@3.15/lib
$ otool -L libv8.dylib
libv8.dylib:
        /usr/local/opt/v8@3.15/lib/libv8.dylib (compatibility version 0.0.0, current version 0.0.0)
        /usr/lib/libstdc++.6.dylib (compatibility version 7.0.0, current version 104.1.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.50.2)
        /usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 489.0.0)

However, the newer versions use libc++:

kevin@cdrv:/usr/local/Cellar/v8/5.1.281.47/lib
$ otool -L libv8.dylib
libv8.dylib:
        /usr/local/opt/v8/lib/libv8.dylib (compatibility version 0.0.0, current version 0.0.0)
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.4.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.0.0)

I'm not yet sure how to convince older versions of libv8 to install using libc++ to test this, but it's possible that the issue is more a mix of libstdc++ + libc++ rather than two different versions of libc++?

@s-u
Copy link

s-u commented Jun 1, 2017

I wouldn't be surprised. If you test the newer one, does that also have the exception problem? Do they behave that same way?

@kevinushey
Copy link

Unfortunately the V8 R package is not compatible with the newer libv8 libraries (I'm guessing due to API changes in libv8 itself). @jeroen, is there any plan to change this in the future?

@jeroen
Copy link
Owner Author

jeroen commented Jun 3, 2017

@kevinushey unfortunately Google doesn't care about non-google users and they change the libv8 API for each minor release. Therefore the only somewhat-stable version of libv8 which is available from all major distributions is the 3.14 ~ 3.15 branch.

Bundling libv8 with the package is also not an option because, again, google doesn't care so they require some annoying non-portable internal build system. So we're stuck on this version for now :/

Perhaps we can suggest a patch for the v8@3.15 homebrew formula to build against libc++?

@jeroen
Copy link
Owner Author

jeroen commented Jul 19, 2017

The upstream homebrew formula has been modified to build against libcxx so hopefully the problem will disappear the next time the V8 binary package for OSX is updated.

@kevinushey
Copy link

Unfortunately I can still reproduce the issue above even with V8 linking to libc++. 😞

@jeroen
Copy link
Owner Author

jeroen commented Jul 19, 2017

Hmm it seems to have solved at least part of the problem for me. But I haven't tested yet with the cran compiler...

@jeroen
Copy link
Owner Author

jeroen commented Aug 15, 2017

@kevinushey is right. Even after rebuilding libv8 with libcxx the problem remains if the V8 package is compiled with the CRAN clang4 compiler.

@jeroen
Copy link
Owner Author

jeroen commented Nov 10, 2017

@mingwandroid does this issue appear with the anaconda compilers as well?

@mingwandroid
Copy link

I'll take a look now. I'll need to add a V8 package first.

@mingwandroid
Copy link

mingwandroid commented Nov 14, 2017

When built with the Anaconda Distribution compilers (and autobrew for v8 itself) I get the SyntaxError so I guess we are free from this problem:

> library('V8')
> ctx <- V8::v8()
> ctx$eval('var foo = {bla}')
Error in context_eval(join(src), private$context) : 
  SyntaxError: Unexpected token }
>

I spotted a number of bugs in clang and ld64 when I was working on building our clang compilers around search paths:

  1. ld64 and clang swap the order of /usr and /usr/local so you can get headers used from one but libraries linked from the other.
  2. /usr/lib/libc++.dylib was getting linked instead of our own libc++.dylib. I fixed that here:
    https://github.com/AnacondaRecipes/aggregate/blob/master/llvm-compilers-feedstock/recipe/530-ld64-add-conda-specific-env-vars-to-modify-lib-search-paths.patch#L88-L108

@mingwandroid
Copy link

mingwandroid commented Nov 14, 2017

The following bash script should reproduce my result (it writes only to /tmp):

#!/usr/bin/env bash

pushd /tmp
CONDA_ROOT=/tmp/mc3

if [[ $(uname) == Darwin ]]; then
  [[ -f Miniconda3-4.3.30.1-MacOSX-x86_64.sh ]] || curl -SLO https://repo.continuum.io/miniconda/Miniconda3-4.3.30.1-MacOSX-x86_64.sh
  [[ -d ${CONDA_ROOT} ]] || bash ./Miniconda3-4.3.30.1-MacOSX-x86_64.sh -bfp ${CONDA_ROOT}
  CONDA_BIN=${CONDA_ROOT}/bin
elif [[ $(uname) == Linux ]]; then
  wget -c https://repo.continuum.io/miniconda/Miniconda3-4.3.30-Linux-x86_64.sh
  [[ -d ${CONDA_ROOT} ]] || bash ./Miniconda3-4.3.30-Linux-x86_64.sh -bfp ${CONDA_ROOT}
  CONDA_BIN=${CONDA_ROOT}/bin
  EXTRA_PKGS=patchelf
else
  wget -c https://repo.continuum.io/miniconda/Miniconda3-4.3.30-Windows-x86_64.exe
  [[ -d ${CONDA_ROOT} ]] || MSYS2_ARG_CONV_EXCL="/wait;/AddToPath;/RegisterPython;/NoRegistry;/S;/D" cmd.exe /c "start /wait \"\" Miniconda3-4.3.30-Windows-x86_64.exe /AddToPath=0 /RegisterPython=0 /NoRegistry=1 /S /D=$(cygpath -w ${CONDA_ROOT})"
  CONDA_BIN=${CONDA_ROOT}/Scripts
fi

. ${CONDA_BIN}/activate

# git and conda-build deps:
conda install -y git beautifulsoup4 conda-verify filelock glob2 jinja2 markupsafe pkginfo pyyaml cachecontrol lockfile conda-build ${EXTRA_PKGS}

# Use our generic cran skeletonizer (we have different ones for different languages and other package repositories):
conda skeleton cran V8 --update-policy=overwrite
# Use Jeroen's AUTOBREW since we do not currently have V8 packages
sed -i.bak 's|export DISABLE_AUTOBREW=1|# export DISABLE_AUTOBREW=1|g' r-v8/build.sh
conda-build r-v8

# Install it in a seprate environment:
conda create -y -n v8-test -c local -c https://repo.continuum.io/pkgs/main r-v8

# Activate that environment
. ${CONDA_BIN}/activate v8-test

# Verify that we get the SyntaxError
R -e "library('V8'); ctx <- V8::v8(); ctx\$eval('var foo = {bla}')"

@jeroen
Copy link
Owner Author

jeroen commented Nov 14, 2017

@mingwandroid the stdc++/libc++ thing is an issue with libv8 and fixed here: Homebrew/homebrew-core@ddfe5c7

@mingwandroid
Copy link

What about #37 (comment) though? You said it was still an issue?

@mingwandroid
Copy link

If not, what are you asking me to try to reproduce?

@jeroen
Copy link
Owner Author

jeroen commented Nov 14, 2017

So which stdlib is now used at runtime? The one that ships with MacOS or one that you bundle with Anaconda?

@mingwandroid
Copy link

Anaconda's own build of libc++ is used exclusively in all of our modern packages. I fixed our clang and ld64 packages to make sure the system one is not used ever.

@jeroen
Copy link
Owner Author

jeroen commented Feb 6, 2019

Hopefully this problem will disappear once we switch to a more recent libv8: #48

@jeroen jeroen closed this as completed Feb 6, 2019
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

4 participants