Php & Ruby Cherry Picks for 3.17.1 (#8632)

* Some more updates to PHP testing infrastructure (#8576) * WIP. * Added build config for all of the tests. * Use ../src/protoc if it is available, for cases where Bazel isn't available. * Added test_php.sh. * Fix for the broken macOS tests. * Move all jobs to use php80 instead of lots of separate jobs. * Only pass -t flag if we are running in a terminal. * Updated php_all job to use new Docker stuff. * Fixed PHP memory leaks and arginfo errors (#8614) * Fixed a bunch of incorrect arginfo and a few incorrect error messages. * Passes mem check test with no leaks! * WIP. * Fix build warning that was causing Bazel build to fail. * Added compatibility code for PHP <8.0. * Added test_valgrind target and made tests Valgrind-clean. * Updated Valgrind test to fail if memory leaks are detected. * Removed intermediate shell script so commands are easier to cut, paste, and modify. * Passing all Valgrind tests! * Hoist addref into ObjCache_Get(). * Removed special case of map descriptors by keying object map on upb_msgdef. * Removed all remaining RETURN_ZVAL() macros. * Removed all explicit reference add/del operations. * Added REFCOUNTING.md to Makefile.am. * Updated upb version and fixed PHP to not get unset message field. (#8621) * Updated upb version and fixed PHP to not get unset message field. * Updated changelog. * Fixed preproc test to handle old versions of Clang withot __has_attribute(). * A second try at fixing __has_attribute(). * Copy __has_attribute() fix to cc file also. * Updated failure list for PHP for fixed test. * Updated version of upb for Ruby (#8624) * Updated upb. * Preserve legacy behavior for unset messages. * Updated failure list. * Updated CHANGES.txt. * Added erroneously-deleted test file. * Fixed condition on compatibility code. * Re-introduced deleted file again, and fixed Rakefile to not delete it. * Fix generation of test protos.
protocolbuffers · May 19, 2021 · 0b87475 · 0b87475
1 parent 65abb64
commit 0b87475
Show file tree

Hide file tree

Showing 33 changed files with 2,706 additions and 2,704 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,3 +1,12 @@
+2021-05-07 version 3.17.1 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript)
+  PHP
+  * Fixed JSON parser to allow multiple values from the same oneof as long as
+    all but one are null.
+
+  Ruby
+  * Fixed JSON parser to allow multiple values from the same oneof as long as
+    all but one are null.
+
 2021-05-07 version 3.17.0 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript)
 
   Protocol Compiler

diff --git a/Makefile.am b/Makefile.am
@@ -801,6 +801,7 @@ objectivec_EXTRA_DIST=                                                       \
 php_EXTRA_DIST=                                                       \
   composer.json                                                       \
   php/README.md                                                       \
+  php/REFCOUNTING.md                                                  \
   php/composer.json                                                   \
   php/ext/google/protobuf/arena.c                                     \
   php/ext/google/protobuf/arena.h                                     \

diff --git a/conformance/failure_list_php_c.txt b/conformance/failure_list_php_c.txt
@@ -1,4 +1,2 @@
 Recommended.Proto2.JsonInput.FieldNameExtension.Validator
 Required.Proto2.JsonInput.StoresDefaultPrimitive.Validator
-Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput
-Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput
diff --git a/conformance/failure_list_ruby.txt b/conformance/failure_list_ruby.txt
@@ -56,5 +56,3 @@ Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT32.PackedInput.UnpackedOu
 Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT32.UnpackedInput.UnpackedOutput.ProtobufOutput
 Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT64.PackedInput.UnpackedOutput.ProtobufOutput
 Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT64.UnpackedInput.UnpackedOutput.ProtobufOutput
-Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput
-Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput
diff --git a/kokoro/linux/php80/build.sh b/kokoro/linux/php80/build.sh
@@ -1,18 +1,17 @@
 #!/bin/bash
 #
-# This is the top-level script we give to Kokoro as the entry point for
-# running the "pull request" project:
-#
-# This script selects a specific Dockerfile (for building a Docker image) and
-# a script to run inside that image.  Then we delegate to the general
-# build_and_run_docker.sh script.
+# This is the entry point for kicking off a Kokoro job. This path is referenced
+# from the .cfg files in this directory.
+
+set -ex
+
+cd $(dirname $0)
 
-# Change to repo root
-cd $(dirname $0)/../../..
+# Most of our tests use a debug build of PHP, but we do one build against an opt
+# php just in case that surfaces anything unexpected.
+../test_php.sh gcr.io/protobuf-build/php/linux:8.0.5-14a06550010c0649bf69b6c9b803c1ca609bbb6d
 
-export DOCKERHUB_ORGANIZATION=protobuftesting
-export DOCKERFILE_DIR=kokoro/linux/dockerfile/test/php80
-export DOCKER_RUN_SCRIPT=kokoro/linux/pull_request_in_docker.sh
-export OUTPUT_DIR=testoutput
-export TEST_SET="php8.0_all"
-./kokoro/linux/build_and_run_docker.sh
+../test_php.sh gcr.io/protobuf-build/php/linux:7.0.33-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
+../test_php.sh gcr.io/protobuf-build/php/linux:7.3.28-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
+../test_php.sh gcr.io/protobuf-build/php/linux:7.4.18-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
+../test_php.sh gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
diff --git a/kokoro/linux/php80/continuous.cfg b/kokoro/linux/php80/continuous.cfg
@@ -2,10 +2,4 @@
 
 # Location of the build script in repository
 build_file: "protobuf/kokoro/linux/php80/build.sh"
-timeout_mins: 120
-
-action {
-  define_artifacts {
-    regex: "**/sponge_log.xml"
-  }
-}
+timeout_mins: 20
diff --git a/kokoro/linux/php80/presubmit.cfg b/kokoro/linux/php80/presubmit.cfg
@@ -2,10 +2,4 @@
 
 # Location of the build script in repository
 build_file: "protobuf/kokoro/linux/php80/build.sh"
-timeout_mins: 120
-
-action {
-  define_artifacts {
-    regex: "**/sponge_log.xml"
-  }
-}
+timeout_mins: 20
diff --git a/kokoro/linux/php_all/build.sh b/kokoro/linux/php_all/build.sh
@@ -1,18 +1,20 @@
 #!/bin/bash
 #
-# This is the top-level script we give to Kokoro as the entry point for
-# running the "pull request" project:
-#
-# This script selects a specific Dockerfile (for building a Docker image) and
-# a script to run inside that image.  Then we delegate to the general
-# build_and_run_docker.sh script.
+# This is the entry point for kicking off a Kokoro job. This path is referenced
+# from the .cfg files in this directory.
+
+set -ex
 
-# Change to repo root
+# Change to repo base.
 cd $(dirname $0)/../../..
 
-export DOCKERHUB_ORGANIZATION=protobuftesting
-export DOCKERFILE_DIR=kokoro/linux/dockerfile/test/php
-export DOCKER_RUN_SCRIPT=kokoro/linux/pull_request_in_docker.sh
-export OUTPUT_DIR=testoutput
-export TEST_SET="php_all"
-./kokoro/linux/build_and_run_docker.sh
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test_valgrind"
+
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.0.33-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.3.28-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.4.18-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+
+# Most of our tests use a debug build of PHP, but we do one build against an opt
+# php just in case that surfaces anything unexpected.
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
diff --git a/php/REFCOUNTING.md b/php/REFCOUNTING.md
@@ -0,0 +1,112 @@
+
+# Refcounting Tips
+
+One of the trickiest parts of the C extension for PHP is getting the refcounting
+right.  These are some notes about the basics of what you should know,
+especially if you're not super familiar with PHP's C API.
+
+These notes cover the same general material as [the Memory Management chapter of
+the PHP internal's
+book](https://www.phpinternalsbook.com/php7/zvals/memory_management.html), but
+calls out some points that were not immediately clear to me.
+
+##  Zvals
+
+In the PHP C API, the `zval` type is roughly analogous to a variable in PHP, eg:
+
+```php
+    // Think of $a as a "zval".
+    $a = [];
+```
+
+The equivalent PHP C code would be:
+
+```c
+    zval a;
+    ZVAL_NEW_ARR(&a);  // Allocates and assigns a new array.
+```
+
+PHP is reference counted, so each variable -- and thus each zval -- will have a
+reference on whatever it points to (unless its holding a data type that isn't
+refcounted at all, like numbers). Since the zval owns a reference, it must be
+explicitly destroyed in order to release this reference.
+
+```c
+    zval a;
+    ZVAL_NEW_ARR(&a);
+
+    // The destructor for a zval, this must be called or the ref will be leaked.
+    zval_ptr_dtor(&a);
+```
+
+Whenever you see a `zval`, you can assume it owns a ref (or is storing a
+non-refcounted type). If you see a `zval*`, which is also quite common, then
+this is *pointing to* something that owns a ref, but it does not own a ref
+itself.
+
+The [`ZVAL_*` family of
+macros](https://github.com/php/php-src/blob/4030a00e8b6453aff929362bf9b25c193f72c94a/Zend/zend_types.h#L883-L1109)
+initializes a `zval` from a specific value type.  A few examples:
+
+* `ZVAL_NULL(&zv)`: initializes the value to `null`
+* `ZVAL_LONG(&zv, 5)`: initializes a `zend_long` (integer) value
+* `ZVAL_ARR(&zv, arr)`: initializes a `zend_array*` value (refcounted)
+* `ZVAL_OBJ(&zv, obj)`: initializes a `zend_object*` value (refcounted)
+
+Note that all of our custom objects (messages, repeated fields, descriptors,
+etc) are `zend_object*`.
+
+The variants that initialize from a refcounted type do *not* increase the
+refcount. This makes them suitable for initializing from a newly-created object:
+
+```c
+    zval zv;
+    ZVAL_OBJ(&zv, CreateObject());
+```
+
+Once in a while, we want to initialize a `zval` while also increasing the
+reference count. For this we can use `ZVAL_OBJ_COPY()`:
+
+```c
+zend_object *some_global;
+
+void GetGlobal(zval *zv) {
+    // We want to create a new ref to an existing object.
+    ZVAL_OBJ_COPY(zv, some_global);
+}
+```
+
+## Transferring references
+
+A `zval`'s ref must be released at some point. While `zval_ptr_dtor()` is the
+simplest way of releasing a ref, it is not the most common (at least in our code
+base). More often, we are returning the `zval` back to PHP from C.
+
+```c
+    zval zv;
+    InitializeOurZval(&zv);
+    // Returns the value of zv to the caller and donates our ref.
+    RETURN_COPY_VALUE(&zv);
+```
+
+The `RETURN_COPY_VALUE()` macro (standard in PHP 8.x, and polyfilled in earlier
+versions) is the most common way we return a value back to PHP, because it
+donates our `zval`'s refcount to the caller, and thus saves us from needing to
+destroy our `zval` explicitly. This is ideal when we have a full `zval` to
+return.
+
+Once in a while we have a `zval*` to return instead. For example when we parse
+parameters to our function and ask for a `zval`, PHP will give us pointers to
+the existing `zval` structures instead of creating new ones.
+
+```c
+    zval *val;
+    if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &val) == FAILURE) {
+      return;
+    }
+    // Returns a copy of this zval, adding a ref in the process.
+    RETURN_COPY(val);
+```
+
+When we use `RETURN_COPY`, the refcount is increased; this is perfect for
+returning a `zval*` when we do not own a ref on it.
diff --git a/php/composer.json b/php/composer.json
@@ -24,6 +24,7 @@
   },
   "scripts": {
     "test_c": "./generate_test_protos.sh && ./tests/compile_extension.sh && php -dextension=ext/google/protobuf/modules/protobuf.so vendor/bin/phpunit --bootstrap tests/force_c_ext.php tests",
+    "test_valgrind": "./generate_test_protos.sh && ./tests/compile_extension.sh && ZEND_DONT_UNLOAD_MODULES=1 USE_ZEND_ALLOC=0 valgrind --leak-check=full --error-exitcode=1 php -dextension=ext/google/protobuf/modules/protobuf.so vendor/bin/phpunit --bootstrap tests/force_c_ext.php tests",
     "test": "./generate_test_protos.sh && vendor/bin/phpunit tests",
     "aggregate_metadata_test": "./generate_test_protos.sh --aggregate_metadata && vendor/bin/phpunit tests"
   }

diff --git a/php/ext/google/protobuf/array.c b/php/ext/google/protobuf/array.c
@@ -337,7 +337,7 @@ PHP_METHOD(RepeatedField, offsetGet) {
 
   msgval = upb_array_get(intern->array, index);
   Convert_UpbToPhp(msgval, &ret, intern->type, &intern->arena);
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 /**
@@ -447,7 +447,7 @@ PHP_METHOD(RepeatedField, count) {
 PHP_METHOD(RepeatedField, getIterator) {
   zval ret;
   RepeatedFieldIter_make(&ret, getThis());
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_construct, 0, 0, 1)
@@ -579,7 +579,7 @@ PHP_METHOD(RepeatedFieldIter, current) {
   msgval = upb_array_get(array, index);
 
   Convert_UpbToPhp(msgval, &ret, field->type, &field->arena);
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 /**

diff --git a/php/ext/google/protobuf/convert.c b/php/ext/google/protobuf/convert.c
@@ -76,7 +76,7 @@ PHP_METHOD(Util, checkMapField) {
                             &val_type, &klass) == FAILURE) {
     return;
   }
-  RETURN_ZVAL(val, 1, 0);
+  RETURN_COPY(val);
 }
 
 // The result of checkRepeatedField() is assigned, so we need to return the
@@ -89,13 +89,18 @@ PHP_METHOD(Util, checkRepeatedField) {
       FAILURE) {
     return;
   }
-  RETURN_ZVAL(val, 1, 0);
+  RETURN_COPY(val);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_checkPrimitive, 0, 0, 1)
   ZEND_ARG_INFO(0, value)
 ZEND_END_ARG_INFO()
 
+ZEND_BEGIN_ARG_INFO_EX(arginfo_checkString, 0, 0, 1)
+  ZEND_ARG_INFO(0, value)
+  ZEND_ARG_INFO(0, check_utf8)
+ZEND_END_ARG_INFO()
+
 ZEND_BEGIN_ARG_INFO_EX(arginfo_checkMessage, 0, 0, 2)
   ZEND_ARG_INFO(0, value)
   ZEND_ARG_INFO(0, class)
@@ -123,15 +128,15 @@ static zend_function_entry util_methods[] = {
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkUint64, arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
-  PHP_ME(Util, checkEnum,   arginfo_checkPrimitive,
+  PHP_ME(Util, checkEnum,   arginfo_checkMessage,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkFloat,  arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkDouble, arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkBool,   arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
-  PHP_ME(Util, checkString, arginfo_checkPrimitive,
+  PHP_ME(Util, checkString, arginfo_checkString,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkBytes,  arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)

diff --git a/php/ext/google/protobuf/convert.h b/php/ext/google/protobuf/convert.h
@@ -60,9 +60,10 @@ bool Convert_PhpToUpbAutoWrap(zval *val, upb_msgval *upb_val, TypeInfo type,
                               upb_arena *arena);
 
 // Converts |upb_val| to a PHP zval according to |type|. This may involve
-// creating a PHP wrapper object. If type == UPB_TYPE_MESSAGE, then |desc| must
-// be the Descriptor for this message type. Any newly created wrapper object
+// creating a PHP wrapper object. Any newly created wrapper object
 // will reference |arena|.
+//
+// The caller owns a reference to the returned value.
 void Convert_UpbToPhp(upb_msgval upb_val, zval *php_val, TypeInfo type,
                       zval *arena);