Make c extension portable for php 7.4 #6968

TeBoring · 2019-12-01T16:47:50Z

TeBoring · 2019-12-01T16:48:28Z

@remicollet, could you help take a look?

remicollet · 2019-12-02T07:35:16Z

Build and load OK

BTW still so much warnings
Ignoring the [-Wunused-variable]:


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c: At top level:
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c:356:8: warning: return type defaults to 'int' [-Wimplicit-int]
  356 | static initialize_persistent_descriptor_pool(TSRMLS_D) {
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c:428:8: warning: return type defaults to 'int' [-Wimplicit-int]
  428 | static cleanup_persistent_descriptor_pool(TSRMLS_D) {
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c: In function 'cleanup_persistent_descriptor_pool':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c:443:1: warning: control reaches end of non-void function [-Wreturn-type]
  443 | }
      | ^
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c: In function 'initialize_persistent_descriptor_pool':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/protobuf.c:376:1: warning: control reaches end of non-void function [-Wreturn-type]
  376 | }
      | ^


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/array.c: In function 'zim_RepeatedFieldIter_current':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/array.c:530:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
  530 |       zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
      |                                            ~^                    ~~~~~
      |                                             |                    |
      |                                             int                  long int
      |                                            %ld
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/array.c:536:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
  536 |       zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
      |                                            ~^                    ~~~~~
      |                                             |                    |
      |                                             int                  long int
      |                                            %ld


dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c: In function 'zim_Util_checkMessage':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:498:65: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'zend_string *' {aka 'struct _zend_string *'} [-Wformat=]
  498 |                             "Given value is not an instance of %s.",
      |                                                                ~^
      |                                                                 |
      |                                                                 char *
  499 |                             klass->name);
      |                             ~~~~~~~~~~~                          
      |                                  |
      |                                  zend_string * {aka struct _zend_string *}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c: In function 'check_repeated_field':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:543:67: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'zend_string *' {aka 'struct _zend_string *'} [-Wformat=]
  543 |                               "Given value is not an instance of %s.",
      |                                                                  ~^
      |                                                                   |
      |                                                                   char *
  544 |                               repeated_field_type->name);
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~            
      |                                                  |
      |                                                  zend_string * {aka struct _zend_string *}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:555:60: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'zend_string * const' {aka 'struct _zend_string * const'} [-Wformat=]
  555 |                               "Expect a repeated field of %s, but %s is given.",
      |                                                           ~^
      |                                                            |
      |                                                            char *
  556 |                               klass->name, intern->msg_ce->name);
      |                               ~~~~~~~~~~~                   
      |                                    |
      |                                    zend_string * const {aka struct _zend_string * const}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:555:68: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'zend_string * const' {aka 'struct _zend_string * const'} [-Wformat=]
  555 |                               "Expect a repeated field of %s, but %s is given.",
      |                                                                   ~^
      |                                                                    |
      |                                                                    char *
  556 |                               klass->name, intern->msg_ce->name);
      |                                            ~~~~~~~~~~~~~~~~~~~~     
      |                                                          |
      |                                                          zend_string * const {aka struct _zend_string * const}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c: In function 'check_map_field':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:619:67: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'zend_string *' {aka 'struct _zend_string *'} [-Wformat=]
  619 |                               "Given value is not an instance of %s.",
      |                                                                  ~^
      |                                                                   |
      |                                                                   char *
  620 |                               map_field_type->name);
      |                               ~~~~~~~~~~~~~~~~~~~~                 
      |                                             |
      |                                             zend_string * {aka struct _zend_string *}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:638:55: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'zend_string * const' {aka 'struct _zend_string * const'} [-Wformat=]
  638 |                               "Expect a map field of %s, but %s is given.",
      |                                                      ~^
      |                                                       |
      |                                                       char *
  639 |                               klass->name, intern->msg_ce->name);
      |                               ~~~~~~~~~~~              
      |                                    |
      |                                    zend_string * const {aka struct _zend_string * const}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/type_check.c:638:63: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'zend_string * const' {aka 'struct _zend_string * const'} [-Wformat=]
  638 |                               "Expect a map field of %s, but %s is given.",
      |                                                              ~^
      |                                                               |
      |                                                               char *
  639 |                               klass->name, intern->msg_ce->name);
      |                                            ~~~~~~~~~~~~~~~~~~~~
      |                                                          |
      |                                                          zend_string * const {aka struct _zend_string * const}


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/def.c: In function 'register_class':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/def.c:960:59: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  960 |         "Generated message class %s hasn't been defined (%s)",
      |                                                          ~^
      |                                                           |
      |                                                           char *


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c: In function 'message_init':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:119:36: warning: assignment to 'zend_object_write_property_t' {aka 'struct _zval_struct * (*)(struct _zval_struct *, struct _zval_struct *, struct _zval_struct *, void **)'} from incompatible pointer type 'void (*)(zval *, zval *, zval *, void **)' {aka 'void (*)(struct _zval_struct *, struct _zval_struct *, struct _zval_struct *, void **)'} [-Wincompatible-pointer-types]
  119 |   message_handlers->write_property = message_set_property;
      |                                    ^


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c: In function 'zim_Field_Cardinality_name':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1158:11: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1158 |           "Enum Google\\Protobuf\\Field_Cardinality has no name "
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1159 |           "defined for value %d.",
 1160 |           value);
      |           ~~~~~
      |           |
      |           zend_long {aka long int}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1159:31: note: format string is defined here
 1159 |           "defined for value %d.",
      |                              ~^
      |                               |
      |                               int
      |                              %ld


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c: In function 'zim_Field_Kind_name':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1293:31: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1293 |                               "Enum Google\\Protobuf\\Field_Kind has no name "
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1294 |                               "defined for value %d.",
 1295 |                               value);
      |                               ~~~~~
      |                               |
      |                               zend_long {aka long int}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1294:51: note: format string is defined here
 1294 |                               "defined for value %d.",
      |                                                  ~^
      |                                                   |
      |                                                   int
      |                                                  %ld


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c: In function 'zim_NullValue_name':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1364:31: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1364 |                               "Enum Google\\Protobuf\\NullValue has no name "
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1365 |                               "defined for value %d.",
 1366 |                               value);
      |                               ~~~~~
      |                               |
      |                               zend_long {aka long int}
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/message.c:1365:51: note: format string is defined here
 1365 |                               "defined for value %d.",
      |                                                  ~^
      |                                                   |
      |                                                   int
      |                                                  %ld


/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/upb.c: In function 'parse':
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/upb.c:11639:5: warning: array subscript -1 is outside array bounds of 'const char[1]' [-Warray-bounds]
11639 |  { p--; {cs = stack[--top];  if ( p == pe )
      |    ~^~
/dev/shm/BUILD/php74-php-pecl-protobuf-3.11.0/NTS/upb.c:8540:19: note: while referencing 'eof_ch'
 8540 | static const char eof_ch = 'e';
      |                   ^~~~~~

remicollet · 2019-12-02T07:37:49Z

class name type, see pr #6226

The write_property one seems critical

From UPGRADING.INTERNALS

  m. The write_property() object handler now returns the assigned value (after
     possible type coercions) rather than void. For extensions, it should
     usually be sufficient to return whatever was passed as the argument.

The file created in php 7.4 is not recognizable by previous versions

TeBoring · 2019-12-04T02:22:00Z

@remicollet addressed your comments. Please take another look.

camry · 2019-12-04T09:41:08Z

When is it updated?

remicollet · 2019-12-04T09:47:53Z

Still some warnings:


/tmp/protobuf/php/ext/google/protobuf/message.c:1177:11: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1177 |           "Enum Google\\Protobuf\\Field_Cardinality has no name "
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1178 |           "defined for value %d.",
 1179 |           value);
      |           ~~~~~
      |           |
      |           zend_long {aka long int}
/tmp/protouf/php/ext/google/protobuf/message.c:1178:31: note: format string is defined here
 1178 |           "defined for value %d.",
      |                              ~^
      |                               |
      |                               int
      |                              %ld
/tmp/protobuf/php/ext/google/protobuf/message.c: In function 'zim_Field_Kind_name':
/tmp/protobuf/php/ext/google/protobuf/message.c:1312:31: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1312 |                               "Enum Google\\Protobuf\\Field_Kind has no name "
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1313 |                               "defined for value %d.",
 1314 |                               value);
      |                               ~~~~~
      |                               |
      |                               zend_long {aka long int}
/tmp/protobuf/php/ext/google/protobuf/message.c:1313:51: note: format string is defined here
 1313 |                               "defined for value %d.",
      |                                                  ~^
      |                                                   |
      |                                                   int
      |                                                  %ld
/tmp/protobuf/php/ext/google/protobuf/message.c: In function 'zim_NullValue_name':
/tmp/protobuf/php/ext/google/protobuf/message.c:1383:31: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1383 |                               "Enum Google\\Protobuf\\NullValue has no name "
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1384 |                               "defined for value %d.",
 1385 |                               value);
      |                               ~~~~~
      |                               |
      |                               zend_long {aka long int}
/tmp/protobuf/php/ext/google/protobuf/message.c:1384:51: note: format string is defined here
 1384 |                               "defined for value %d.",
      |                                                  ~^
      |                                                   |
      |                                                   int
      |                                                  %ld
/tmp/protobuf/php/ext/google/protobuf/message.c: In function 'zim_Syntax_name':
/tmp/protobuf/php/ext/google/protobuf/message.c:1440:31: warning: format '%d' expects argument of type 'int', but argument 4 has type 'zend_long' {aka 'long int'} [-Wformat=]
 1440 |                               "Enum Google\\Protobuf\\Syntax has no name "
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1441 |                               "defined for value %d.",
 1442 |                               value);
      |                               ~~~~~
      |                               |
      |                               zend_long {aka long int}
/tmp/protobuf/php/ext/google/protobuf/message.c:1441:51: note: format string is defined here
 1441 |                               "defined for value %d.",
      |                                                  ~^
      |                                                   |
      |                                                   int
      |                                                  %ld
/tmp/protobuf/php/ext/google/protobuf/protobuf.c: In function 'zm_startup_protobuf':
/tmp/protobuf/php/ext/google/protobuf/protobuf.c:457:13: warning: 'v.val' is used uninitialized in this function [-Wuninitialized]
  457 |   upb_value v;
      |             ^
tmp/protobuf/php/ext/google/protobuf/storage.c: In function 'layout_get_oneof_case':
/tmp/protobuf/php/ext/google/protobuf/storage.c:1174:26: warning: 'first_field' may be used uninitialized in this function [-Wmaybe-uninitialized]
 1174 |   uint32_t* oneof_case = slot_oneof_case(layout, storage, first_field);
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/protobuf/php/ext/google/protobuf/upb.c: In function 'parse':
/tmp/protobuf/php/ext/google/protobuf/upb.c:11639:5: warning: array subscript -1 is outside array bounds of 'const char[1]' [-Warray-bounds]
11639 |  { p--; {cs = stack[--top];  if ( p == pe )
      |    ~^~
/tmp/protobuf/php/ext/google/protobuf/upb.c:8540:19: note: while referencing 'eof_ch'
 8540 | static const char eof_ch = 'e';
      |                   ^~~~~~

remicollet · 2019-12-04T09:49:52Z

For the -Wformat issue on zend_long var, I recommend to use the ZEND_LONG_FMT macro (PHP 7 only)

TeBoring · 2019-12-05T03:26:16Z

How do you compile the extension (32bit or 64bit, which php version, which compile flag)?
I cannot reproduce the warnings.

remicollet · 2019-12-05T07:11:08Z

How do you compile the extension (32bit or 64bit, which php version, which compile flag)?

PHP 7.4.0, GCC 9.2.1, x86_64 with default RPM build flags:
--O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection

remicollet · 2019-12-05T07:15:21Z

Also, see https://github.com/xdebug/xdebug/blob/master/config.m4#L36 which is an example of how to enable most sanity checks during the build.

TeBoring · 2019-12-07T01:00:59Z

Fixed compiler warnings. Warnings in upb needs to be fixed in upb repository.
PTAL

nesl247 · 2019-12-10T15:42:48Z

Any idea when this will be released?

TeBoring · 2019-12-10T15:44:57Z

EOW for now.

…

On Tue, Dec 10, 2019 at 07:42 Harrison Heck ***@***.***> wrote: Any idea when this will be released? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6968?email_source=notifications&email_token=ABHUPZO6F67VFMMCBPRWE5TQX62HVA5CNFSM4JTMSCFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPWEZY#issuecomment-564093543>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHUPZOH7AJA6SBLJ7PSADTQX62HVANCNFSM4JTMSCFA> .

Make c extension portable for php 7.4

15dc0f4

TeBoring added php kokoro:run labels Dec 1, 2019

googlebot added the cla: yes label Dec 1, 2019

protobuf-kokoro removed the kokoro:run label Dec 1, 2019

Fix conformance tests

32d41f0

TeBoring added the kokoro:run label Dec 3, 2019

protobuf-kokoro removed the kokoro:run label Dec 3, 2019

Fix comments

5b075fd

TeBoring added the kokoro:force-run label Dec 3, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 3, 2019

Fix 32-bit

451a046

TeBoring added the kokoro:force-run label Dec 3, 2019

protobuf-kokoro removed kokoro:force-run labels Dec 3, 2019

Update conformance failure list

f52a076

TeBoring added the kokoro:force-run label Dec 3, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 3, 2019

Fix compiler warnings

f06c6f6

TeBoring added the kokoro:force-run label Dec 3, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 3, 2019

Cleanup configure created by phpize

37614cf

The file created in php 7.4 is not recognizable by previous versions

TeBoring added the kokoro:force-run label Dec 4, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 4, 2019

Fix conformance tests for 64-bit php

4a56758

TeBoring added the kokoro:force-run label Dec 4, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 4, 2019

Fix conformance test

3a71cde

TeBoring added the kokoro:force-run label Dec 4, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 4, 2019

Fix compile warning

3505012

TeBoring added the kokoro:force-run label Dec 5, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 5, 2019

Fix compile warnings

5150589

TeBoring added the kokoro:force-run label Dec 7, 2019

protobuf-kokoro removed the kokoro:force-run label Dec 7, 2019

DRoet mentioned this pull request Dec 8, 2019

PHP 7.4 zend_mm_heap corrupted #6985

Closed

stanley-cheung approved these changes Dec 10, 2019

View reviewed changes

TeBoring merged commit a66423f into protocolbuffers:3.11.x Dec 10, 2019

TeBoring deleted the php-7.4-fix branch December 10, 2019 01:02

Make c extension portable for php 7.4 #6968

Make c extension portable for php 7.4 #6968

Uh oh!

Conversation

TeBoring commented Dec 1, 2019

Uh oh!

TeBoring commented Dec 1, 2019

Uh oh!

remicollet commented Dec 2, 2019

Uh oh!

remicollet commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TeBoring commented Dec 4, 2019

Uh oh!

camry commented Dec 4, 2019

Uh oh!

remicollet commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remicollet commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TeBoring commented Dec 5, 2019

Uh oh!

remicollet commented Dec 5, 2019

Uh oh!

remicollet commented Dec 5, 2019

Uh oh!

TeBoring commented Dec 7, 2019

Uh oh!

nesl247 commented Dec 10, 2019

Uh oh!

TeBoring commented Dec 10, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

remicollet commented Dec 2, 2019 •

edited

Loading

remicollet commented Dec 4, 2019 •

edited

Loading

remicollet commented Dec 4, 2019 •

edited

Loading