[PHP8 incompability] Incorrect location information for PHP project (Laravel)

[luislavena]

Hello folks,

Originally shared this on Slack to confirm was something new, posting here now for better visibility:

I wanted to try it out Pyroscope on a Laravel application and since most of the work I do is within containers, I setup some sandbox for it (here is the branch where I added it). It uses the Docker's official PHP 8.0 (bullseye) + some extensions. I made sure that the container has SYS_PTRACE capabilities.

You can simply:

$ cp .env.example .env

$ docker compose build

$ docker compose up

And open http://localhost:4040 to access Pyroscope.

When running, found that the location information contains some garbage and not the real file locations of the calls:

I decided to manually build phpspy (since this image includes the compiler) and phpspy was able to report correctly for the same application the correct location information.

Thought first this was due Docker's PHP version not having debug information, but standalone phpspy working make me think might be something else.

I haven't tested if this is reproducible outside of containers (since I don't have any development tool installed)

Let me know if you need additional details.

Last but not least, Pyroscope looks amazing! Thank you for sharing it with the world! ❤️ ❤️ ❤️

[petethepig]

@AdrK Could you look into this?

[AdrK]

Hi, Thanks for reporting, I am onto it right now. Firstly, I managed to reproduce this and now trying to find where the data gets corrupted.

From the first look it seems that the phpspy itself is giving us rubbish.

I have added this simple line to the phpspy fork that we are using (basically printing out the stacktraces that are sent to Pyroscope): https://github.com/pyroscope-io/phpspy/commit/5508a6eea88003191332b6e534494f89fbcf2339

And I can already see the rubbish: phpspy output: <internal> - Illuminate\Foundation\Console\Kernel::handle;$:0 - Illuminate\Console\Application::run;�:1162023040 - Symfony\Component\Console\Application::run;�:1161061248 - Symfony\Component\Console\Application::doRun;�:1160208288 - Symfony\Component\Console\Application::doRunCommand;�*G�:0 - Illuminate\Console\Command::run;:1148395760 - Symfony\Component\Console\Command\Command::run;:0 - Illuminate\Console\Command::execute;<internal> - Illuminate\Container\Container::call;0"8f~U:0 - Illuminate\Container\BoundMethod::call;<internal> - Illuminate\Container\BoundMethod::callBoundMethod;<internal> - Illuminate\Container\Util::unwrapIfClosure;<internal> - Illuminate\Container\BoundMethod::Illuminate\Container\{closure};t:0 - Illuminate\Foundation\Console\ServeCommand::handle;<internal> - usleep;

[AdrK]

Going further:

https://github.com/pyroscope-io/phpspy/commit/ec9ca41505ffd90a7ccc48bd236f3f026b336a74

[luislavena]

Great findings @AdrK !!! I tested against master branch of phpspy directly, perhaps there are changes in between the fork and that?

[AdrK]

Can you please tell which exact SHA of phpspy have you been testing?

[luislavena]

@AdrK on the train right now without computer, but I believe latest head of master branch: https://github.com/adsr/phpspy/commit/f4083341025607e7f067b6ad52852b61d17cb6c4

Will confirm if a few hours, thank you!

[AdrK]

Oh, actually there were so many changes that it is hard to compare. We haven't update the fork for long time... When you reach home, can you possibly test the phpspy, as you tested with the master branch, but using be3abd72e8e2dd5dd4e61008fcd702f90c6eb238 SHA? This is the SHA from which we have forked.

[luislavena]

Not sure if this helps: https://github.com/adsr/phpspy/compare/f577b71b6eaebece00160b77d2ecf40a8cf0ded4...f4083341025607e7f067b6ad52852b61d17cb6c4

Was not able to see anything in particular about dealing with file location.

[luislavena]

(got that commit by looking at the latest changes on pyroscope fork made but the author of the library)

[AdrK]

Oh sorry, wrong SHA. The proper one, from which we forked is: f577b71b6eaebece00160b77d2ecf40a8cf0ded4

Also we build the phpspy in some special way: USE_ZEND=1 make CFLAGS="-DUSE_DIRECT"

EDIT: At the end of the day, the changes comes down to one file: addr_objdump.c. And that doesn't fix anything, investigating further.

[AdrK]

There are some cases when the filename is not available, and we have kind of a protection for it, but it only works when the filename is actually empty if (zfunc.type == 2 && zfunc.op_array.filename != NULL) {...}

For some reason, in this env, the filename contains rubbish, so this check doesn't actually work. Now the question is, why there is no filename or rubbish in the filename in the first place.

[luislavena]

@AdrK this might help, within the container environment when running with pyroscope, started to see the following:

template-laravel-twill-app-1        | web    | Starting Laravel development server: http://0.0.0.0:80
template-laravel-twill-app-1        | web    | [Tue Mar 29 12:22:42 2022] PHP 8.0.17 Development Server (http://0.0.0.0:80) started
template-laravel-twill-app-1        | web    | copy_proc_mem_direct: Failed to copy zfunc; err=I/O error raddr=0x7fcb00000308 size=216
template-laravel-twill-app-1        | web    | copy_proc_mem_direct: Failed to copy zfunc; err=I/O error raddr=0x7fcb00000308 size=216

(that is the output when running with docker compose and overmind with the Procfile)

In the diff on the original phpspy there are some buffer sizes changes, which could be connected to that.

I lack the Go expertise to debug this further, but happy to help if you let me know what else I can test.

Once again thank you for your support and responses. ❤️ ❤️ ❤️

[AdrK]

Hi, Sorry for long delay. Would you be feasible for you to test with PHP 7.3 or 7.4? I was trying to, but it seems that the dev.dockerfile is using some custom image.

EDIT: I run our custom unit tests for pyroscope api in our forked phpspy with php 8.1 and they pass. They also pass in ghcr.io/luislavena/hydrofoil-php:8.0 container.

[AdrK]

ok, I think I have found it. It was quite simple. Basically, fix in Pyroscope repo looks like this:

diff --git a/Dockerfile b/Dockerfile
index f4e3ac55..56ef1ac2 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -34,7 +34,7 @@ RUN mv /opt/rustdeps/target/$(uname -m)-unknown-linux-musl/release/librustdeps.a
 # | |         | |       | |     __/ |
 # |_|         |_|       |_|    |___/
 
-FROM php:7.3-fpm-alpine3.13 as phpspy-builder
+FROM php:8.0.13-fpm-alpine3.13 as phpspy-builder
 RUN apk add --update alpine-sdk
 COPY Makefile Makefile
 RUN mkdir -p third_party/phpspy

I think we have to figure out a way to deliver for multiple php versions. After this small change:

[luislavena]

Hello @AdrK, thank you for your patience, I was out the last few weeks.

I can confirm that switching to PHP 7.3 for the custom setup I used, it correctly shows the file:line information:

Looking at phpspy_trace_tpl.c, the functions and data structures to use are specific of each version of PHP, so having one library/executable that works across all the versions might not be possible.

Not sure if the approach used by xdebug or blackfire, install as PHP extension, might be worth exploring.

Let me know any way I can help with.

Once again, thank you for your support and responses! ❤️ ❤️ ❤️

[luislavena]

Hello @Rperry2174, would you mind point me to the place this has been completed? I was not able to see recent changes in relation to PHP support.

Perhaps with the move to Grafana that idea/support has been dropped?

Thank you in advance for your response. ❤️ ❤️ ❤️