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

Fix i386 release build warning #16730

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Nov 8, 2024

--disable-debug is added temporarily to catch the error in CI.

https://github.com/php/php-src/actions/runs/11734433568/job/32690495546#logs

image

@cmb69
Copy link
Member

cmb69 commented Nov 8, 2024

php-src/main/getopt.c

Lines 87 to 100 in 3b609ab

if ((argv[*optind][0] == '-') && (argv[*optind][1] == '-')) {
const char *pos;
size_t arg_end = strlen(argv[*optind])-1;
/* '--' indicates end of args if not followed by a known long option name */
if (argv[*optind][2] == '\0') {
(*optind)++;
return(EOF);
}
arg_start = 2;
/* Check for <arg>=<val> */
if ((pos = php_memnstr(&argv[*optind][arg_start], "=", 1, argv[*optind]+arg_end)) != NULL) {

If only -- is given, arg_start==2 and arg_end==1.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Nov 8, 2024

@cmb69 I think the issue is the hard-coded 1 needle length in php_memnstr(). php_memnstr() passes needle_len-2 to memcmp, which would underflow, but at that point needle_len == 1 was already handled. When inlining php_memnstr(), I would expect GCC to discard the needle_len > 1 branch altogether, but for some reason it does not.

Maybe I'm on the wrong track. I don't have a 32-bit build set up locally.

@cmb69
Copy link
Member

cmb69 commented Nov 8, 2024

Why do we even use php_memnstr() here instead of memchr()? disregard

@iluuu1994
Copy link
Member Author

iluuu1994 commented Nov 8, 2024

That's a good question. We don't use php_memnstr() with a hard-coded needle_len=1 anywhere else, which is likely why this issue only occurs here.

@cmb69
Copy link
Member

cmb69 commented Nov 8, 2024

Oops, I'm confused. Shouldn't do too much when bisecting. ;)

Anyhow, relevant commit: 024fc2c (2007!)

@iluuu1994
Copy link
Member Author

Your suggestion seems to work 🙂 Thanks!

@cmb69
Copy link
Member

cmb69 commented Nov 9, 2024

Certainly no ABI break here.

@cmb69 cmb69 removed the ABI break label Nov 9, 2024
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

The change to getopt.c looks good to me (might make sense to apply to lower branches too; at least PHP-8.4).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants