-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fixing darwin native backend [WIP] #2254
Conversation
3abca0c
to
914f7e6
Compare
How should I proceed with these issues from DeepSource? They all seem not to be related to this PR, not sure why they creep up now. Is there a way to re-trigger the Travis CI tests? I guess cirrus only checks Linux, right? |
Ignore them if it's not something you added.
No, Travis CI recently changed their plan for open source software and now we run out of credits in a couple of days. This is an ongoing problem at the moment.
Freebsd, actually. |
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.
Initial pass done, few nits and questions.
I do want to say a huge thank you for taking this on, however. Cleaning up and fixing the native backend has been on my list for a long time now, and I'm really happy you've submitted this patch to improve things, so thanks a lot!
ca00a41
to
484b495
Compare
393883b
to
5c938b9
Compare
Unfortunately, I am running into some issues with implementing trace functionality, i.e. attaching to an existing PID under As you can see here, Go forces PIE under While launching an executable, we can disable ASLR, but when attaching to existing processes, we can not :( |
Good news, I managed to retrieve the start of the |
50f9f9e
to
52bec15
Compare
Looks like CI is passing with this patch! I've been off for the holidays the last 2 weeks so I'm in catch up mode, but I will review this patch this week. My current line of thinking is to cut the initial v1.6 release with lldb-server based darwin/arm64 support and then review & merge this, let it simmer for a while, let folks test it, and then include it when we do a v1.6.x release. |
Sounds like a good idea! In the long run you might want to decide whether to keep the native backend or not. Now at least there is Darwin/amd64 support through the lldb server. |
Added correct entitlements and replaced the fork_exec mechanism to a POSIX spawn. This now enables the launch of the inferiour and succesfully calls task_for_pid(). The following functionality has been tested: * Setting breakpoints * Thread continue * Thread cpu instruction step * Reading go routine * Printing stracktrace * Reading variables / locals There are still a couple of tests from the 'basic' test suite that fail: * TestStacktrace - which is weird, because the same functionality works in CLI * TestStacktrace2 crashes * TestNextGeneral * TestNextConcurrent * TestNextFunctionReturnDefer
50c3523
to
1b2dbbf
Compare
After a long absence on my part, I have decided the rebase this against the current master. I have also reverted some changes that made this the default since we now have support using LLDB on darwin/arm64 now. How should we proceed with this @derekparker? I did not have have time yet to check what has changed in the last couple of months. What is your future plan with regards to this? |
7fa2e89
to
933e539
Compare
@derekparker Just a quick question. Is this something still to be considered? Then I would try to find the time to resolve the merge conflicts. Or should we just abandon the native Mac backend alltogether? |
@oxisto I would very much like to continue supporting the native Mac backend. If you can find the time to address the conflicts and continue hacking on this it would be greatly appreciated! |
@oxisto Also, I apologize for not responding to your previous comment, I do not want to give you the impression that this is not important or appreciated work, sometimes I have every intention to reply and follow up but things fall through the cracks. If you rebase this and ping me I will make sure to provide prompt reviews and feedback. |
No worries :) I just wanted to make sure that this is still on the roadmap before I put more effort in. |
Closing stale PR, however @oxisto I'm still very interested in seeing this land. If you don't have the time I can try taking this over myself. |
@oxisto if you are still open to completing this work, feel free to rebase this PR and reopen it and I will make it a priority to review and give feedback. |
Unfortunately I do not see any free time this year to tackle this :( Maybe next year. |
Added correct entitlements and replaced the fork_exec mechanism to a POSIX spawn. This now enables the launch of the inferior and successfully calls task_for_pid().
The following functionality has been tested:
If finished, this fixes #2246 and it fixes #1112. The reason, I chose to fix #2246 this way is because I am running into more blocking issues using Apple's debugserver and the gdbserver approach. Generally, I would be fine with the gdbserver as default backend on darwin amd64 and the native approach on darwin arm64.