Skip to content

Fixed compilation warnings on big endian system #5373

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fixed compilation warnings on big endian system #5373

wants to merge 1 commit into from

Conversation

vibhutisawant
Copy link
Contributor

This PR is related to bug-79431

Following changes were added as make command fails on big endian architecture(s390x). These changes work fine for little endian architecture too. More details about the same are mentioned in the bug-79431.

cmb69
cmb69 reviewed Apr 12, 2020
View reviewed changes
ext/ffi/ffi_parser.c Outdated
@@ -313,7 +313,7 @@ static int get_skip_sym(void) {
int ch;
int ret;
int accept = -1;
const unsigned char *accept_pos;
const unsigned char *accept_pos = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is moot, since ffi_parser.c is generated from ffi.g, so the change would likely have to be made at https://github.com/dstogov/llk/blob/70b057f49948a1f5bb37cc5cb1a324605c3903e3/llk_c.php#L262.

nikic
nikic reviewed Apr 12, 2020
View reviewed changes
ext/pdo/pdo_stmt.c Outdated
@@ -750,7 +750,7 @@ static int do_fetch(pdo_stmt_t *stmt, int do_bind, zval *return_value, enum pdo_
{
int flags, idx, old_arg_count = 0;
zend_class_entry *ce = NULL, *old_ce = NULL;
zval grp_val, *pgrp, retval, old_ctor_args = {{0}, {{0}}, {0}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was recently added in 6e40ec7 to fix another warning... cc @Girgias

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a bug in GCC as to why it reports the missing initializer after digging around a bit:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80454
I wonder if just doing {0} would fix both warnings as this should NULL initialize the whole struct from my (albeit limited) understanding.
Will try to apply this change and run on CI in #5151

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be cleaner to omit the initializer, and to do ZVAL_UNDEF(old_ctor_args) instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 You won't be believe me, but doing that is going to trigger another compiler warning (-Wmaybe-uninitialized). No matter what you do, there's always a warning...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried just using {0} and Clang complains...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So got a fix which doesn't emit a warning on any platform: 31f3b78
Many thanks to @booti386 for helping me out here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Girgias The above mentioned fix throws error on little as well as big endian arch without removing the --enable-werror flag from travis/compile.sh.

Girgias
Girgias reviewed Apr 12, 2020
View reviewed changes
Girgias
Girgias reviewed Apr 12, 2020
View reviewed changes
ext/pdo/pdo_stmt.c Outdated
@@ -750,7 +750,7 @@ static int do_fetch(pdo_stmt_t *stmt, int do_bind, zval *return_value, enum pdo_
{
int flags, idx, old_arg_count = 0;
zend_class_entry *ce = NULL, *old_ce = NULL;
zval grp_val, *pgrp, retval, old_ctor_args = {{0}, {{0}}, {0}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried just using {0} and Clang complains...

@vibhutisawant vibhutisawant requested a review from nikic April 13, 2020 15:53
@Girgias
Copy link
Member

Girgias commented Apr 13, 2020

Commited 446724b and 8300458 so that I can open #5382 to see test failures which arise in some extensions due to the architecture being big endian.

@vibhutisawant if you could rebase onto master and force push that would be great :)

@vibhutisawant
Copy link
Contributor Author

@Girgias I have done a rebase with master and updated the PR.

Girgias
Girgias approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although if you could PR the upstream parser lib which generates the FFI parser which would prevent this from reappearing at a later stage.

@vibhutisawant vibhutisawant mentioned this pull request Apr 14, 2020
Closed
@vibhutisawant
Copy link
Contributor Author

@Girgias the warning thrown due to uninitialized accept_pos is an incorrect warning as @dstogov mentioned in the issue above.Tried using gcc 9.3 to build PHP, however it resulted in new warnings. Also the travis job uses gcc 5.4.0. Since its an intermittent warning, can it be ignored?

Or can we set -Wno-error=maybe-uninitialized flag in case of s390x?

@Girgias
Copy link
Member

Girgias commented Apr 15, 2020

@Girgias the warning thrown due to uninitialized accept_pos is an incorrect warning as @dstogov mentioned in the issue above.Tried using gcc 9.3 to build PHP, however it resulted in new warnings. Also the travis job uses gcc 5.4.0. Since its an intermittent warning, can it be ignored?

Or can we set -Wno-error=maybe-uninitialized flag in case of s390x?

Seems reasonable, I've know GCC8 introduced new warnings in -Wextra that needed to be fixed after I merge #5151
I've only compiled PHP with GCC7 so mind providing the compiler log in a gist?

Also can you rebase and drop the FFI change so that I can merge and close this. :)

@vibhutisawant
Copy link
Contributor Author

@Girgias Links to the build logs :
gcc 9.3.0
gcc 7.4.0

Also by using gcc 7.4.0, there were no compile time warnings observed on s390x.
However, travis is using gcc 5.4.0 which throws above mentioned warnings, so how do we go about this?

@nikic
Copy link
Member

nikic commented Apr 15, 2020 •
edited
Loading

@vibhutisawant There are no warnings on the Travis build in #5382, so it's not a problem for our CI at least.

@vibhutisawant
Copy link
Contributor Author

@nikic

Also the travis job uses gcc 5.4.0. Since its an intermittent warning, can it be ignored?

I have observed the uninitialized accept_pos warning when i tried building the master branch and the latest commit was 8e5df60

@php-pulls php-pulls closed this in 481caf1 Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 left review comments

@Girgias Girgias Girgias approved these changes

@nikic nikic Awaiting requested review from nikic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vibhutisawant @Girgias @nikic @cmb69