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

On MacOS, always wrap "dtrace" invocation in "arch -arch $native_arch" #304

Merged
merged 7 commits into from
Feb 12, 2024
Merged

On MacOS, always wrap "dtrace" invocation in "arch -arch $native_arch" #304

merged 7 commits into from
Feb 12, 2024

Conversation

zbentley
Copy link
Contributor

@zbentley zbentley commented Feb 4, 2024

Fixes #302 by always running dtrace as the native architecture. This is accomplished by adding another layer of wrapping in addition to sudo: the arch command forces its spawned children to run as the specified architecture.

Initially I was doing a lot more in this PR (parsing out the architecture of the target binary to be traced and making dtrace run as that), but it turned out to be unnecessary since is prevented from tracing cross-architecture binaries on MacOS in all circumstances.

Since the dtrace binary itself is multi-arch, spawning it inside the arch -arch arm64 command will never fail, but if users do something like cargo flamegraph --root - --target x86_64-apple-darwin they'll get an error from dtrace itself saying "no" regardless of whether this PR has been applied or not.

@zbentley
Copy link
Contributor Author

There's an option to simplify this code and remove the uname dependency by running arch -64 or arch -32 which prefers the native 64- or 32-bit architecture. However, that would necessarily encode a preference for a specific bit width, since probing the program being profiled for its architecture would be a fair amount more code (see diff as of aa9be61 for an example of how that might look, it's a lot).

Is saying "always invoke native 64-bit dtrace" or "always invoke native dtrace at a bit architecture matching the flamegraph binary" an acceptable tradeoff? If so, I can make that diff.

@djc
Copy link
Contributor

djc commented Feb 12, 2024

"always invoke native dtrace at a bit architecture matching the flamegraph binary" an acceptable tradeoff? If so, I can make that diff.

This seems like a reasonable option to me?

@zbentley
Copy link
Contributor Author

This seems like a reasonable option to me?

Done!

@djc djc merged commit 1d7be18 into flamegraph-rs:main Feb 12, 2024
5 checks passed
@djc
Copy link
Contributor

djc commented Feb 12, 2024

Thanks!

@zbentley
Copy link
Contributor Author

Thanks!

Thanks for reviewing!

@zbentley zbentley deleted the fix_302 branch February 12, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants