Skip to content

Conversation

@dstogov
Copy link
Member

@dstogov dstogov commented Jan 9, 2024

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
Copy link
Member

@ndossche ndossche left a comment

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
Copy link
Member Author

dstogov commented Jan 9, 2024

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
Copy link
Member Author

dstogov commented Jan 9, 2024

(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
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