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

Enable function call injection in Delve for linux/ppc64le #3449

Merged
merged 10 commits into from
Sep 21, 2023

Conversation

archanaravindar
Copy link
Contributor

@archanaravindar archanaravindar commented Jul 25, 2023

Using the function call injection feature to be enabled in Go runtime for linux/ppc64le, delve can also make use of this feature on linux/ppc64le
The CL in gerrit which has been opened is as below
https://go-review.googlesource.com/c/go/+/512575
The debug call function tests dont work on PIE mode as a result they are skipped. Another PR will address this.

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this PR makes function call injection fail on every other architecture.

pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/linutil/regs_ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/proc/linutil/regs_ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/proc/linutil/regs_ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/terminal/command_test.go Outdated Show resolved Hide resolved
@aarzilli
Copy link
Member

aarzilli commented Aug 1, 2023

See test failures.

pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/terminal/command_test.go Outdated Show resolved Hide resolved
service/test/integration2_test.go Outdated Show resolved Hide resolved
pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved
pkg/proc/variables_test.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/fncall.go Outdated Show resolved Hide resolved
pkg/proc/native/registers_linux_ppc64le.go Outdated Show resolved Hide resolved
pkg/proc/test/support.go Outdated Show resolved Hide resolved
pkg/proc/variables_test.go Show resolved Hide resolved
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

LGTM

Just waiting on https://go-review.googlesource.com/c/go/+/512575 to merge first to ensure we don't have to fix anything up after the fact.

@aarzilli
Copy link
Member

LGTM

Just waiting on https://go-review.googlesource.com/c/go/+/512575 to merge first to ensure we don't have to fix anything up after the fact.

There are a few other complications with testing this:

  1. the ppc64le agents still don't work
  2. something like https://go-review.googlesource.com/c/go/+/515637 needs to be merged in the rungime as well (see also my last comment there)
  3. something like Various fixes for go 1.22 #3455 needs to be merged in delve

@derekparker
Copy link
Member

@archanaravindar can you fix the rebase conflicts and then we can merge?

@derekparker derekparker merged commit ebc3e61 into go-delve:master Sep 21, 2023
1 check passed
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.

3 participants