Merge pull request #115 from bzz/docs-and-leaks

Docs and leaks

Author: bzz

CONTRIBUTING.md

@@ -0,0 +1,103 @@
+# Contribution Guidelines
+
+As all source{d} projects, this project follows the
+[source{d} Contributing Guidelines](https://github.com/src-d/guide/blob/master/engineering/documents/CONTRIBUTING.md).
+
+
+# Developer documentation
+
+This section has more extended documentation for a brave developer willing to
+contribute by diving into and debugging the native glue code in `src/main/native`.
+
+## Build libuast with debug symbols
+This is not strictly necessary, but will help you to navigate though the stack-traces.
+
+Clone [libuast](https://github.com/bblfsh/libuast) locally and from the project root do:
+```
+CGO_ENABLED=1 go build -buildmode=c-archive -gcflags "-N -l" -o=libuast.a ./src
+mv libuast.a ../scala-client/src/main/resources/libuast
+```
+
+Sadly, on non-linux OSes you might still experience issues with CGO due to https://github.com/golang/go/issues/5221.
+
+
+## Build libscalauast with debug symbols
+Compiler flags need to be `-g -O0` used instead of `-fPIC -O2` that is used for releases:
+```
+$ g++ -shared -Wall -g -std=c++11 -O0 \
+      -I/usr/include \
+      "-I${JAVA_HOME}/include" \
+      "-I${JAVA_HOME}/include/${platform}" \
+      -Isrc/main/resources/libuast \
+      -o "src/main/resources/lib/libscalauast${platform_ext}" \
+      src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc \
+      src/main/native/jni_utils.cc src/main/resources/libuast/libuast.a
+```
+
+## Run a single test under debugger
+To run a single test from CLI one can:
+
+```
+./sbt 'testOnly org.bblfsh.client.v2.libuast.IteratorNativeTest -- -z "Native UAST iterator should return non-empty results on decoded objects"'
+```
+
+When using `lldb`, the classpath needs to be manually set for the `java` executable:
+
+```
+PATH="/usr/bin:$PATH" lldb -- java -ea -Xcheck:jni -Djava.library.path=src/main/resources -cp "target/classes:target/test-classes:src/main/resources:${HOME}/.ivy2/cache/org.scalatest/scalatest_2.11/bundles/scalatest_2.11-3.0.1.jar:${HOME}/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.11.jar:${HOME}/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11/bundles/scala-xml_2.11-1.0.5.jar:${HOME}/.ivy2/cache/org.scalactic/scalactic_2.11/bundles/scalactic_2.11-3.0.1.jar:${HOME}/.ivy2/cache/commons-io/commons-io/jars/commons-io-2.5.jar:build/bblfsh-client-assembly-2.0.0-SNAPSHOT.jar:target/*" \
+  org.scalatest.tools.Runner \
+  -s org.bblfsh.client.v2.libuast.IteratorNativeTest \
+  -z "Native UAST iterator" \
+  -f iterator-native-test.txt
+```
+Actual test output will be saved in `iterator-native-test.txt`.
+
+## When inside the debugger
+
+These instructions are for `lldb`, but the steps should be similar in `gdb`.
+To load the debug symbols do:
+
+```
+run
+continue
+continue
+target symbols add src/main/resources/lib/libscalauast.dylib.dSYM
+```
+
+If that does not load the symbols, you have to make sure `libscalauast` library has
+already been loaded to the process by `target modules list`.
+If the library loads but symbols are not correctly displayed, it probably means
+the library is at the wrong filesystem path.
+Stop the debugger and do `rm -rf ./target/classes/lib/libscalauast.dylib*`
+
+To actually debug, set a breakpoint like:
+```
+br s -r Java_org_bblfsh_client_v2_libuast_Libuast_00024UastIter_nativeNext
+run
+c
+```
+
+Check the stack trace and print values do:
+```
+bt
+p *node
+```
+
+Check [official LLDB documentation](https://lldb.llvm.org/use/map.html) for more
+use cases and instructions.
+
+## More tips on JNI debugging
+
+A small curated list of really useful resources on Go&JNI debugging:
+ - https://github.com/facebook/rocksdb/wiki/JNI-Debugging
+ - https://gist.github.com/dwbuiten/c9865c4afb38f482702e#2-debugging-tips
+ - Go and LLDB http://ribrdb.github.io/lldb/
+
+## JNI details
+
+Here are some resources to understand the JNI machinery and its best practices:
+ - https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html
+ - https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html
+ - https://developer.android.com/training/articles/perf-jni
+ - https://www.artima.com/insidejvm/ed2/jvm9.html
+ - http://blog.jamesdbloom.com/JVMInternals.html

README.md

@@ -1,16 +1,16 @@
 ## Babelfish Scala client [![Build Status](https://travis-ci.org/bblfsh/scala-client.svg?branch=master)](https://travis-ci.org/bblfsh/scala-client)
 
-This a Scala/JNI implementation of the [Babelfish](https://doc.bblf.sh/) client.
+This is a Scala/JNI implementation of the [Babelfish](https://doc.bblf.sh/) client.
 It uses [ScalaPB](https://scalapb.github.io/grpc.html) for Protobuf/gRPC code
 generation and [libuast](https://github.com/bblfsh/libuast) for XPath queries.
 
 ### Status
 
-Current `scala-client` v1.x only supports bblfsh protocol and UASTv1.
+The latest `scala-client` *v2.x* supports the [UASTv2 protocol](https://doc.bblf.sh/uast/uast-specification-v2.html).
 
 ### Installation
 
-#### Manual
+#### Building from sources
 ```
 git clone https://github.com/bblfsh/scala-client.git
 cd scala-client
@@ -32,12 +32,14 @@ If the build fails because it can't find the `jni.h` header file, run it with:
 
 Changing the JDK directory to the one right for your system.
 
+For more developer documentation please check our [CONTRIBUTING](./CONTRIBUTING.md) guideline.
+
 #### Apache Maven
 
 The `bblfsh-client` package is available thorugh [Maven
 central](http://search.maven.org/#search%7Cga%7C1%7Cbblfsh), so it can be easily
 added as a dependency in various package management systems.  Examples of how to
-handle it for most commons systems are included below; for other systems just look
+handle it for most common systems are included below; for other systems just look
 at Maven central's dependency information.
 
 ```xml
@@ -73,7 +75,7 @@ docker run --privileged --rm -it -p 9432:9432 --name bblfsh bblfsh/bblfshd
 ```
 
 Please, read the [getting started](https://doc.bblf.sh/using-babelfish/getting-started.html)
-guide to learn more about how to use and deploy a bblfsh server.
+guide to learn more about how to use and deploy a bblfsh server, install language drivers, etc.
 
 API
 ```scala
@@ -103,7 +105,7 @@ java -jar build/bblfsh-client-assembly-*.jar -f <file.py>
 or if you want to use a XPath query:
 
 ```
-java -jar build/bblfsh-client-assembly-*.jar -f <file.py> -q "//Import[@roleImport]"
+java -jar build/bblfsh-client-assembly-*.jar -f <file.py> -q "//uast:Import"
 ```
 
 Please read the [Babelfish clients](https://doc.bblf.sh/user/language-clients.html)

build.sbt

@@ -166,8 +166,8 @@ def compileUnix(sourceFiles: String) = {
   }
 
   val osName = System.getProperty("os.name").toLowerCase()
-  if (osName.contains("mac os x")) { // TODO(bzz): change back to '-fPIC -O2' for release
-    val cmd:String = "g++ -shared -Wall -g -std=c++11 " +
+  if (osName.contains("mac os x")) {
+    val cmd:String = "g++ -shared -Wall -fPIC -O2 -std=c++11 " +
       "-I/usr/include " +
       "-I" + javaHome + "/include/ " +
       "-I" + javaHome + "/include/darwin " +

src/main/native/jni_utils.cc

@@ -62,7 +62,7 @@ const char METHOD_JITER_INIT[] = "(Lorg/bblfsh/client/v2/JNode;IJJ)V";
 // Field signatures
 const char FIELD_ITER_NODE[] = "Ljava/lang/Object;";
 
-// TODO(bzz): cache classes&methods in JNI_OnLoad should speed this up
+// TODO(#114): cache classes&methods in JNI_OnLoad should speed this up
 void checkJvmException(std::string msg) {
   JNIEnv *env = getJNIEnv();
   auto err = env->ExceptionOccurred();
@@ -224,14 +224,14 @@ jmethodID MethodID(JNIEnv *env, const char *method, const char *signature,
 }
 
 jint IntMethod(JNIEnv *env, const char *method, const char *signature,
-               const char *className, const jobject *object) {
+               const char *className, const jobject object) {
   jmethodID mId = MethodID(env, method, signature, className);
   checkJvmException(std::string("failed to get method ")
                         .append(className)
                         .append(".")
                         .append(method));
 
-  jint res = env->CallIntMethod(*object, mId);
+  jint res = env->CallIntMethod(object, mId);
   checkJvmException(std::string("failed to call method ")
                         .append(className)
                         .append(".")
@@ -243,7 +243,7 @@ jint IntMethod(JNIEnv *env, const char *method, const char *signature,
 }
 
 jobject ObjectMethod(JNIEnv *env, const char *method, const char *signature,
-                     const char *className, const jobject *object, ...) {
+                     const char *className, const jobject object, ...) {
   jmethodID mId = MethodID(env, method, signature, className);
   checkJvmException(std::string("failed to get method ")
                         .append(className)
@@ -252,7 +252,7 @@ jobject ObjectMethod(JNIEnv *env, const char *method, const char *signature,
 
   va_list varargs;
   va_start(varargs, object);
-  jobject res = env->CallObjectMethodV(*object, mId, varargs);
+  jobject res = env->CallObjectMethodV(object, mId, varargs);
   va_end(varargs);
   checkJvmException(std::string("failed to get varargs for ")
                         .append(className)

src/main/native/jni_utils.h

@@ -71,12 +71,12 @@ jmethodID MethodID(JNIEnv *, const char *, const char *, const char *);
 // Calls a method of the given class or interface name that returns an Int.
 // The method is determined by its name and signature.
 jint IntMethod(JNIEnv *, const char *, const char *, const char *,
-               const jobject *);
+               const jobject);
 
 // Calls a method of the given class or interface name that returns an Object.
 // The method is determined by its name and signature.
 jobject ObjectMethod(JNIEnv *, const char *, const char *, const char *,
-                     const jobject *, ...);
+                     const jobject, ...);
 
 // Constructs new object the given class name and throws it to JVM.
 //

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc

@@ -276,7 +276,7 @@ class Node : public uast::Node<Node *> {
       const char methodName[] = "str";
       JNIEnv *env = getJNIEnv();
       jstring jstr = (jstring)ObjectMethod(
-          env, methodName, "()Ljava/lang/String;", CLS_JSTR, &obj);
+          env, methodName, "()Ljava/lang/String;", CLS_JSTR, obj);
 
       const char *utf = env->GetStringUTFChars(jstr, 0);
       str = new std::string(utf);
@@ -339,7 +339,7 @@ class Node : public uast::Node<Node *> {
     return value;
   }
   size_t Size() {
-    jint size = IntMethod(getJNIEnv(), "size", "()I", CLS_JNODE, &obj);
+    jint size = IntMethod(getJNIEnv(), "size", "()I", CLS_JNODE, obj);
     assert(int32_t(size) >= 0);
 
     return size;
@@ -349,7 +349,7 @@ class Node : public uast::Node<Node *> {
 
     JNIEnv *env = getJNIEnv();
     jstring key = (jstring)ObjectMethod(env, "keyAt", METHOD_JNODE_KEY_AT,
-                                        CLS_JNODE, &obj, i);
+                                        CLS_JNODE, obj, i);
 
     const char *k = env->GetStringUTFChars(key, 0);
     std::string *s = new std::string(k);
@@ -362,7 +362,7 @@ class Node : public uast::Node<Node *> {
 
     JNIEnv *env = getJNIEnv();
     jobject val =
-        ObjectMethod(env, "valueAt", METHOD_JNODE_VALUE_AT, CLS_JNODE, &obj, i);
+        ObjectMethod(env, "valueAt", METHOD_JNODE_VALUE_AT, CLS_JNODE, obj, i);
     return lookupOrCreate(env->NewGlobalRef(val));  // new ref
   }
 
@@ -375,7 +375,7 @@ class Node : public uast::Node<Node *> {
       v = NewJavaObject(env, CLS_JNULL, "()V");
     }
 
-    ObjectMethod(getJNIEnv(), "add", METHOD_JARR_ADD, CLS_JARR, &obj, v);
+    ObjectMethod(getJNIEnv(), "add", METHOD_JARR_ADD, CLS_JARR, obj, v);
     checkJvmException(std::string("failed to call ")
                           .append(CLS_JARR)
                           .append(".add() from Node::SetValue()"));
@@ -391,7 +391,7 @@ class Node : public uast::Node<Node *> {
 
     jstring k = env->NewStringUTF(key.data());
 
-    ObjectMethod(env, "add", METHOD_JOBJ_ADD, CLS_JOBJ, &obj, k, v);
+    ObjectMethod(env, "add", METHOD_JOBJ_ADD, CLS_JOBJ, obj, k, v);
     checkJvmException(
         std::string("failed to call JObject.add() from Node::SetKeyValue(")
             .append(key)
@@ -633,7 +633,7 @@ Java_org_bblfsh_client_v2_libuast_Libuast_00024UastIter_nativeInit(
   }
 
   // global ref will be deleted by Interface destructor on ctx deletion
-  auto it = ctx->Iterate(env->NewGlobalRef(jnode), (TreeOrder)order);
+  auto it = ctx->Iterate(jnode, (TreeOrder)order);
 
   // this.iter = it;
   setHandle<uast::Iterator<Node *>>(env, self, it, "iter");
@@ -754,8 +754,7 @@ JNIEXPORT jobject JNICALL Java_org_bblfsh_client_v2_Context_filter(
 
   uast::Iterator<Node *> *it = nullptr;
   try {
-    // global ref will be deleted by Interface destructor on ctx deletion
-    it = ctx->Filter(env->NewGlobalRef(jnode), query);
+    it = ctx->Filter(jnode, query);
   } catch (const std::exception &e) {
     ThrowByName(env, CLS_RE, e.what());
     return nullptr;
@@ -819,8 +818,10 @@ JNIEXPORT jobject JNICALL Java_org_bblfsh_client_v2_ContextExt_encode(
 JNIEXPORT void JNICALL
 Java_org_bblfsh_client_v2_ContextExt_dispose(JNIEnv *env, jobject self) {
   ContextExt *p = getHandle<ContextExt>(env, self, nativeContext);
-  setHandle<ContextExt>(env, self, 0, nativeContext);
-  delete p;
+  if (!p) {
+    delete p;
+    setHandle<ContextExt>(env, self, 0, nativeContext);
+  }
 }
 
 // ==========================================