benfred / py-spy

Sampling profiler for Python programs
MIT License
12.14k stars 400 forks source link

Fix string copy test by using predictable struct memory layout for `AllocatedPyASCIIObject` #574

Closed zanieb closed 1 year ago

zanieb commented 1 year ago

Resolves failing test test_copy_string:

---- python_data_access::tests::test_copy_string stdout ----
copied: "\0\0\0\0\0\0\0\0\0\0\0\0\0"
thread 'python_data_access::tests::test_copy_string' panicked at 'assertion failed: `(left == right)`
  left: `"\0\0\0\0\0\0\0\0\0\0\0\0\0"`,
 right: `"function_name"`', src/python_data_access.rs:490:9

or

 ---- python_data_access::tests::test_copy_string stdout ----
thread 'python_data_access::tests::test_copy_string' panicked at 'called `Result::unwrap()`
 on an `Err` value: invalid utf-8 sequence of 1 bytes from index 2', src/python_data_access.rs:370:58

which occurs due to a failure to copy test data during into a AllocatedPyASCIIObject during to_asciiobject due to optimization of the struct layout by Rust. We can ask for a C-compatible layout to get a consistent layout for pointer arithmetic.

See passing builds at https://github.com/madkinsz/py-spy/pull/3 See failing builds at https://github.com/madkinsz/py-spy/pull/1 or https://github.com/benfred/py-spy/pull/541

zanieb commented 1 year ago

Failing freebsd build addressed in https://github.com/benfred/py-spy/pull/576

benfred commented 1 year ago

thanks for the fix! it looks good - aside from that I'd like to maintain testing on ARM here .

zanieb commented 1 year ago

thanks for the fix! it looks good - aside from that I'd like to maintain testing on ARM here .

Ah sorry about that looks like I didn't do a great job cherry-picking it back over here

benfred commented 1 year ago

no worries! thanks again for the fix -