Skip to content

[PHP next] Support --enable-debug-assertions configure option on Windows #15544

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

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Aug 22, 2024

This has been introduced for non Windows system years ago[1] mainly to
improve fuzzer SAPI. While this is still not available on Windows, the
option can still make sense, particularly for CI runs or other
environments where full debugging support would be useless, but where
enabled assertions would be helpful.

[1] c4e2ca6


Thanks to @arnaud-lb for telling me about this configuration option!

Note that a couple of ext/ffi test are failing if this is enabled, due to

function ffi_get_php_dll_name()
{
if (PHP_OS_FAMILY === 'Windows') {
return "php" . PHP_MAJOR_VERSION . (PHP_ZTS ? "ts" : "") . (PHP_DEBUG ? "_debug" : "") . ".dll";
} else {
return null;
}
}

One may argue that the userland PHP_DEBUG shouldn't be set to true for --enable-debug-assertions, but since it is on non Windows systems, it's probably okay.

(The issue with the other failing test has been resolved in this PR.)

Still unresolved:

  • fix the failing FFI tests
  • check whether everything works as intended for phpize builds
  • check clang on Windows

Oh, and while I was working on this, I've noticed that setting or not setting the NDEBUG macro shouldn't be necessary, due to

php-src/main/php.h

Lines 112 to 118 in c79e723

#if PHP_DEBUG
#undef NDEBUG
#else
#ifndef NDEBUG
#define NDEBUG
#endif
#endif

However, that doesn't affect Zend/, most notably zend_vm_execute.h will not grab that, so maybe this should be revised.

And I'm not sure about

// `php.h` sets `NDEBUG` when not `PHP_DEBUG` which will make `assert()` from
// assert.h a no-op. In order to have `assert()` working on NDEBUG builds, we
// undefine `NDEBUG` and re-include assert.h
#undef NDEBUG
#include "assert.h"

While that worked for me, can we be sure that the inclusion of assert.h isn't guarded, so that this will not necessarily work? It might make more sense to run (some) CI jobs with --enable-debug-assertions instead. /cc @realFlowControl

And I'm not sure what to do with the NDebug and _DEBUG symbols; the latter is supposed to be automatically set by the compiler, so there is likely no need to set it manually. And regarding the former (which is apparently there since the beginning of the new Windows build infrastructure, 05b9b20), I have no idea what it's supposed to do (maybe support for some ancient compiler?)

cmb69 added 2 commits August 22, 2024 18:07
This has been introduced for non Windows system years ago[1] mainly to
improve fuzzer SAPI.  While this is still not available on Windows, the
option can still make sense, particularly for CI runs or other
environments where full debugging support would be useless, but where
enabled assertions would be helpful.

[1] <php@c4e2ca6>
`PHP_DEBUG` actually doesn't tell whether have a debug build; the
MSVC specific `_DEBUG` does, though.
@cmb69 cmb69 changed the title Support --enable-debug-assertions configure option on Windows [PHP next] Support --enable-debug-assertions configure option on Windows Sep 21, 2024
@cmb69 cmb69 mentioned this pull request Sep 22, 2024
5 tasks
@cmb69
Copy link
Member Author

cmb69 commented Dec 7, 2024

I'm closing this, since --enable-sanitizer is now supported for MSVC and that implicitly also enables debug assertions (c9cc89c).

@cmb69 cmb69 closed this Dec 7, 2024
@cmb69 cmb69 deleted the cmb/debug-assertions-win branch December 7, 2024 16:06
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.

1 participant