-
Notifications
You must be signed in to change notification settings - Fork 571
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
Fix macOS compilation #5325
Fix macOS compilation #5325
Conversation
To be clear, the
By the way, could you provide a link or an update to clarify what is the state of DynamoRIO on macOS? Thank you! |
An extra indirection layer is necessary to force preprocessor expansion in cases where DECLARE_FUNC argument is itself a macro.
Ready for merging on my side. Test failure on ci-windows is apparently caused by a timeout (lag?) on the CI side and not by the changes. |
Thank for the contribution. For future updates please avoid force-pushing (as noted in the docs https://dynamorio.org/page_contributing.html, https://dynamorio.org/page_code_reviews.html) as it ruins multi-step reviews (can't see just new changes since review; messes up comment history). The final squash-and-merge into master will clean up all the commits. |
I was going to say, please update our automated tests in We should still go to 11: if you update the test does it work with your changes? |
It does compile with the new Xcode, but the test suite is indeed partly broken. Would you like me to disable the deprecation warning with a pragma? |
Also, I failed to find this issue earlier, but now I see that this pull request resolves #4344. |
OK, so maybe we need to separate that out since it likely takes some work, and just put in the compilation fix in this PR.
Sorry, what warning is this? |
Tests fail, because vfork is deprecated on macOS. Can either silence the warning or disable the test.
Can do, but maybe first see how bad it is. Fixing the vfork one should be very straightforward. |
Disabling the test on mac seems reasonable.
Sure, sounds good. Just didn't want to pile too much work at once here. |
I had to additionally fix syscall-mod test, which had a crash with modern clang. There are two issues there:
Since one needs to define a complete list of clobbered registers, i.e. modified implicitly or explicitly aside the outputs, in the inline asm, I also tried to fix the code for other architectures. I do not have the AArch64 ABI nearby to be sure, and the i386 ABI does not define the int 0x80 ABI, so that would be Linux-specific, but the changes for the other architectures are good to cover at least case (2). Hopefully we will be good now :-) |
Yes, looks good now. code_api|client.winxfer failed on windows 32-bit, but I have not changed anything related, so it must be a sporadic failure. |
Yes known flake #4732
Biggest missing thing on Mac is a private loader. Without it, large tools like Dr. Memory have their resources and libraries collide with large applications. So right now only small/isolated tools or small apps work. Another significant missing piece is Mac on AArch64 (e.g., M1) but it may not be too much work since Linux support on AArch64 is pretty good: probably mostly signal state and other things. At a high level, what's missing are new developers to own Mac support and spend time on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits and minor suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you for the patch.
add to allowlist |
run arm tests |
Tested on macOS 12.2 with the following compilation commands:
Dependencies like snappy were installed with Homebrew.