Skip to content

Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind - #13099

Merged
dstogov merged 1 commit into
php:PHP-8.2from
dstogov:noipa
Jan 9, 2024
Merged

dstogov merged 1 commit into
php:PHP-8.2from
dstogov:noipa

Conversation

@dstogov

@dstogov dstogov commented Jan 9, 2024

Copy link
Copy Markdown
Member

This is a more safely way to fix GH-9068

…al() function that may be overriden for valgrind

This is a more safely way to fix phpGH-9068

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The reason I did not fix it this way is because I was afraid this could negatively impact performance of normal executions, while the issue only happens when people want to debug using Valgrind.

In any case, this solution seems safer and looks correct. I tested and it works. Thanks.

@dstogov

dstogov commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

The reason I did not fix it this way is because I was afraid this could negatively impact performance of normal executions, while the issue only happens when people want to debug using Valgrind.

Yeah, I understood, but the degradation is really small (valgrind shows 0.0004% more instructions on 100 requests to wordpress). Also your solution wasn't complete as it didn't prevent inlining (I saw it with CLANG).

In any case, this solution seems safer and looks correct. I tested and it works. Thanks.

Thanks for taking a look.

@dstogov
dstogov merged commit 6339938 into php:PHP-8.2 Jan 9, 2024
dstogov added a commit that referenced this pull request Jan 9, 2024
* PHP-8.2:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
dstogov added a commit that referenced this pull request Jan 9, 2024
* PHP-8.3:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
@dstogov

dstogov commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

(valgrind shows 0.0004% more instructions on 100 requests to wordpress).

Measuring this with valgrind doesn't make any sense of course, but analysing the call traces, I saw that zend_string_equal_val() was never called from zend_string.h at all.

shivammathur pushed a commit to shivammathur/php-src that referenced this pull request Oct 13, 2025
…al() function that may be overriden for valgrind (php#13099)

This is a more safely way to fix phpGH-9068
shivammathur pushed a commit to shivammathur/php-src that referenced this pull request Oct 13, 2025
…al() function that may be overriden for valgrind (php#13099)

This is a more safely way to fix phpGH-9068
iluuu1994 added a commit that referenced this pull request Oct 15, 2025
* PHP-8.1:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
  Skip lc_ctype_inheritance.phpt on macos 15+
iluuu1994 added a commit that referenced this pull request Oct 15, 2025
* PHP-8.2:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
  Skip lc_ctype_inheritance.phpt on macos 15+
iluuu1994 added a commit that referenced this pull request Oct 15, 2025
* PHP-8.3:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
  Skip lc_ctype_inheritance.phpt on macos 15+
iluuu1994 added a commit that referenced this pull request Oct 15, 2025
* PHP-8.4:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
  Skip lc_ctype_inheritance.phpt on macos 15+
iluuu1994 added a commit that referenced this pull request Oct 15, 2025
* PHP-8.5:
  Disable inlining and inter-procedure-analyses for zend_string_equal_val() function that may be overriden for valgrind (#13099)
  Skip lc_ctype_inheritance.phpt on macos 15+
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