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

Add integration tests #294

Merged
merged 23 commits into from
Jan 26, 2024
Merged

Conversation

danog
Copy link
Collaborator

@danog danog commented Nov 24, 2023

Reverts #293

@danog
Copy link
Collaborator Author

danog commented Nov 29, 2023

Don't have a mac os machine so can't easily debug the mac os failures, would be really happy if someone could give a try...

@joehoyle
Copy link
Collaborator

joehoyle commented Dec 1, 2023

I am able to reproduce this on PHP 8.0 with macos, but only the debug build of shivammathur, it works fine on php@8.0.

I dug pretty deep bit didn't get a definitive answer. It seems PHP is not able to retrieve the classname of the return type of the function. I verified the FunctionEntry is set the with correct type:

[src/zend/_type.rs:83] ptr_ = 0x000000013960acb0
[src/zend/_type.rs:85] &*(ptr_ as *mut ZendStr) = Ok(
    "TestClass",
)
[src/builders/function.rs:151] &args = [
    _zend_internal_arg_info {
        name: 0x0000000000000000,
        type_: zend_type {
            ptr: 0x000000013960acb0,
            type_mask: 8388608,
        },
        default_value: 0x0000000000000000,
    },
]

The "extra" ArgInfo for a function which contains the return type is good. The type_mask is correctly _ZEND_TYPE_NAME_BIT and the ptr points to a peristant zendstr with the class name.

I can see when the function is called, the _zend_function in the ExecuteData looks like the zend_type is incorrect at that point. As I understand it, the arg_info for the return type is offset -1 from the function.common.arg_info (looking at the path in zend_verify_return_error. The type_mask looks correct at that point, but the ptr is pointing to an invalid ZendStr from what I can see:

[tests/src/lib.rs:143] args = _zend_arg_info {
    name: 0x0000000000000000,
    type_: zend_type {
        ptr: 0x0000000141e0bfc0,
        type_mask: 8388608,
    },
    default_value: 0x0000000000000000,
}

The ptr for type_.ptr:

[tests/src/lib.rs:142] &*(args.type_.ptr as *const ZendStr) = Ok(
    "\u{1}",
)

I'm not sure if this is the right rabbit hole to be going down. I don't see anything that should make the debug build fail, it could be that the ZendStr is somehow getting freed, perhaps the debug build is changing the memory behaviour.

@danog
Copy link
Collaborator Author

danog commented Dec 1, 2023

Oh so the mac os shivammathur php build is actually a debug build, that could actually explain some of the (unrelated, but similarly occurring due to memory corruption) failures I was getting locally with a Linux debug build, unreproducible in the CI....

Might be worth using a debug build for all CI tasks.

@joehoyle
Copy link
Collaborator

joehoyle commented Dec 1, 2023

Oh I see why it's only being triggered on the Debug build, it seems this check is only made for internal functions if ZEND_EDBUG:

#if ZEND_DEBUG
static ZEND_COLD void zend_verify_internal_return_error(const zend_function *zf, zval *value)
{
	...

static ZEND_COLD void zend_verify_void_return_error(const zend_function *zf, const char *returned_msg, const char *returned_kind)
{
	...
}

static bool zend_verify_internal_return_type(zend_function *zf, zval *ret)
{
	...
}
#endif

@joehoyle
Copy link
Collaborator

joehoyle commented Dec 1, 2023

Yeah I'd imagine we should be running all the test envs with a debug build in that case. i think you may be able to reproduce if run you against a debug build on linux?

@joehoyle
Copy link
Collaborator

joehoyle commented Dec 1, 2023

FYI we are setting the PHP build to debug on macos here: https://github.com/davidcole1340/ext-php-rs/blob/master/.github/workflows/build.yml#L48

@danog
Copy link
Collaborator Author

danog commented Dec 1, 2023

Oh I see why it's only being triggered on the Debug build, it seems this check is only made for internal functions if ZEND_EDBUG:
think you may be able to reproduce if run you against a debug build on linux?

Indeed, I encountered the exact same issue previously on a linux debug build precisely due to that type check for return types, which lead to the discovery of a bug in ZendType (a simple C string was used instead of a zend string for the type name), which I fixed in 86c9074, but then the same issue started occurring again here, only on Mac OS builds, and not on a local Linux debug build (which has different issues, namely #290 + a segfault when running library tests probably due to memory corruption as for #290).

FYI we are setting the PHP build to debug on macos here: https://github.com/davidcole1340/ext-php-rs/blob/master/.github/workflows/build.yml#L48

Actually shouldn't that enable debug builds for all OSes?

@joehoyle
Copy link
Collaborator

joehoyle commented Dec 1, 2023

Hmm ok I think 86c9074#diff-758fcaf4f7ca5c65a01fb257423dc04af5d266f93547a89f2559bec5410b2f1aL85 might be the key. To also test this I can do:

$r = new ReflectionFunction('test_class');
var_dump( $r->getReturnType()->getName() );

That gives me string(1) "" which I think is the corrupt. When I change it back to a CString ptr: CString::new(class_name).ok()?.into_raw() as *mut c_void, then I get the correct return type:

string(9) "TestClass"

@danog
Copy link
Collaborator Author

danog commented Jan 26, 2024

The issue was caused by the fact that php 8.2 and below want a C string in the arginfo to then convert it internally to a zend string, but php 8.3 wants a C string only if the literal flag is set, and a zend string otherwise.

@danog danog marked this pull request as ready for review January 26, 2024 13:42
@danog danog merged commit 8985cb2 into master Jan 26, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants