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

Fixing darwin native backend [WIP] #2254

Closed
wants to merge 3 commits into from

Conversation

oxisto
Copy link
Contributor

@oxisto oxisto commented Dec 7, 2020

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:

  • Setting breakpoints
  • Thread continue
  • Thread cpu instruction step
  • Reading go routine
  • Printing stracktrace
  • Reading variables / locals

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.

@derekparker derekparker self-requested a review December 8, 2020 16:59
pkg/proc/native/threads_darwin.go Outdated Show resolved Hide resolved
pkg/proc/native/threads_darwin_arm64.go Outdated Show resolved Hide resolved
pkg/proc/native/registers_darwin_arm64.go Outdated Show resolved Hide resolved
pkg/proc/native/registers_darwin_amd64.go Outdated Show resolved Hide resolved
service/test/integration2_test.go Outdated Show resolved Hide resolved
@oxisto
Copy link
Contributor Author

oxisto commented Dec 9, 2020

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?

@aarzilli
Copy link
Member

aarzilli commented Dec 9, 2020

How should I proceed with these issues from DeepSource?

Ignore them if it's not something you added.

Is there a way to re-trigger the Travis CI tests?

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.

I guess cirrus only checks Linux, right?

Freebsd, actually.

Copy link
Member

@derekparker derekparker left a 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!

_scripts/testsign Outdated Show resolved Hide resolved
pkg/proc/native/exec_darwin.c Show resolved Hide resolved
pkg/proc/native/exec_darwin.c Outdated Show resolved Hide resolved
pkg/proc/native/proc_darwin.c Outdated Show resolved Hide resolved
pkg/proc/native/proc_darwin.c Outdated Show resolved Hide resolved
@oxisto
Copy link
Contributor Author

oxisto commented Dec 29, 2020

Unfortunately, I am running into some issues with implementing trace functionality, i.e. attaching to an existing PID under darwin/arm64.

As you can see here, Go forces PIE under darwin/arm64:
https://github.com/golang/go/blob/cb84d831c956026f477b52c9f8a7c1ed2b2724ad/src/cmd/link/internal/ld/config.go#L39-L40

While launching an executable, we can disable ASLR, but when attaching to existing processes, we can not :(

@oxisto
Copy link
Contributor Author

oxisto commented Dec 30, 2020

Unfortunately, I am running into some issues with implementing trace functionality, i.e. attaching to an existing PID under darwin/arm64.

As you can see here, Go forces PIE under darwin/arm64:
https://github.com/golang/go/blob/cb84d831c956026f477b52c9f8a7c1ed2b2724ad/src/cmd/link/internal/ld/config.go#L39-L40

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 __TEXT section using mach_vm_region, so debugging with ASLR enabled works now!

@derekparker
Copy link
Member

derekparker commented Jan 5, 2021

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.

@oxisto
Copy link
Contributor Author

oxisto commented Jan 6, 2021

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.

@derekparker derekparker added this to the v1.6.1 milestone Jan 29, 2021
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
@oxisto
Copy link
Contributor Author

oxisto commented Apr 18, 2021

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?

@derekparker derekparker modified the milestones: v1.6.1, 1.6.2 May 18, 2021
@derekparker derekparker modified the milestones: v1.6.2, v1.7.1 Jul 6, 2021
@derekparker derekparker removed this from the v1.7.1 milestone Aug 17, 2021
@derekparker derekparker added this to the v1.7.2 milestone Aug 17, 2021
@derekparker derekparker modified the milestones: v1.7.2, v1.7.3 Sep 17, 2021
@oxisto
Copy link
Contributor Author

oxisto commented Oct 13, 2021

@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?

@derekparker
Copy link
Member

@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!

@derekparker
Copy link
Member

@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.

@oxisto
Copy link
Contributor Author

oxisto commented Oct 13, 2021

@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.

@derekparker derekparker modified the milestones: v1.7.3, v1.7.4 Oct 30, 2021
@derekparker derekparker modified the milestones: v1.7.4, v1.8.1 Jan 25, 2022
@derekparker derekparker modified the milestones: v1.8.1, v1.8.2 Feb 3, 2022
@derekparker derekparker modified the milestones: v1.8.2, Unplanned Mar 1, 2022
@derekparker
Copy link
Member

derekparker commented Nov 16, 2022

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.

@derekparker
Copy link
Member

@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.

@oxisto
Copy link
Contributor Author

oxisto commented Nov 17, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS's native backend doesn't work Apple M1 ARM support
4 participants