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

proc: use stack machine to evaluate expressions #3508

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

aarzilli
Copy link
Member

This commit splits expression evaluation into two parts. The first part (in
pkg/proc/evalop/evalcompile.go) "compiles" as ast.Expr into a list of
instructions (defined in pkg/proc/evalop/ops.go) for a stack machine
(defined by proc.(*evalStack)).

The second part is a stack machine (implemented by proc.(*EvalScope).eval
and proc.(*EvalScope).evalOne) that has two modes of operation: in the
main mode it executes inteructions from the list (by calling evalOne), in
the second mode it executes the call injection protocol by calling
funcCallStep repeatedly until it either the protocol finishes, needs more
input from the stack machine (to set call arguments) or fails.

This approach has several benefits:

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 review done.

This is amazing work!

pkg/dwarf/godwarf/helpers.go Outdated Show resolved Hide resolved
pkg/dwarf/godwarf/helpers.go Outdated Show resolved Hide resolved
pkg/proc/eval.go Outdated Show resolved Hide resolved
pkg/proc/eval.go Show resolved Hide resolved
pkg/proc/eval.go Outdated Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Outdated Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Outdated Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Show resolved Hide resolved
@derekparker
Copy link
Member

Just out of curiosity have you ran benchmarks on the existing implementation and the new one?

@aarzilli
Copy link
Member Author

aarzilli commented Oct 9, 2023

Just out of curiosity have you ran benchmarks on the existing implementation and the new one?

No but I don't expect it to matter much. Last time I did benchmark this code it was completely dominated by system calls.

This commit splits expression evaluation into two parts. The first part (in
pkg/proc/evalop/evalcompile.go) "compiles" as ast.Expr into a list of
instructions (defined in pkg/proc/evalop/ops.go) for a stack machine
(defined by `proc.(*evalStack)`).
The second part is a stack machine (implemented by `proc.(*EvalScope).eval`
and `proc.(*EvalScope).evalOne`) that has two modes of operation: in the
main mode it executes inteructions from the list (by calling `evalOne`), in
the second mode it executes the call injection protocol by calling
`funcCallStep` repeatedly until it either the protocol finishes, needs more
input from the stack machine (to set call arguments) or fails.

This approach has several benefits:

- it is now possible to remove the goroutine we use to evaluate expression
  and the channel used to communicate with the Continue loop.
- every time we resume the target to execute the call injection protocol we
  need to update several local variables to match the changed state of the
  target, this is now done at the top level of the evaluation loop instead of
  being hidden inside a recurisive evaluator
- using runtime.Pin to pin addresses returned by an injected call would
  allow us to use a more natural evaluation order for function calls, which
  would solve some bugs go-delve#3310, allow users to inspect values returned by a
  call injection go-delve#1599 and allow implementing some other features go-delve#1465. Doing
  this with the recursive evaluator, while keeping backwards compatibility
  with versions of Go that do not have runtime.Pin is very hard. However after
  this change we can simply conditionally change how compileFunctionCall works
  and add some opcodes.
@aarzilli
Copy link
Member Author

aarzilli commented Oct 9, 2023

For some reason I have pushed to my evalrefactor branch but github has decided not to pick up my changes in this PR. I'm not sure what to do, for now I'll wait and see if it fixes itself.

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.

A few more small things. This overall looks great though.

pkg/proc/eval.go Show resolved Hide resolved
pkg/proc/evalop/evalcompile.go Outdated Show resolved Hide resolved
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

@derekparker derekparker merged commit 2c70023 into go-delve:master Oct 17, 2023
1 of 2 checks passed
kakkoyun added a commit to parca-dev/parca-agent that referenced this pull request Jan 8, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/go-delve/delve](https://togithub.com/go-delve/delve) |
`v1.21.2` -> `v1.22.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgo-delve%2fdelve/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgo-delve%2fdelve/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgo-delve%2fdelve/v1.21.2/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgo-delve%2fdelve/v1.21.2/v1.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>go-delve/delve (github.com/go-delve/delve)</summary>

###
[`v1.22.0`](https://togithub.com/go-delve/delve/releases/tag/v1.22.0)

[Compare
Source](https://togithub.com/go-delve/delve/compare/v1.21.2...v1.22.0)

#### What's Changed

- all: remove obsolete build tags by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3513
- service/dap: add the concrete type to interface type by
[@&#8203;suzmue](https://togithub.com/suzmue) in
[go-delve/delve#3510
- service/dap: fix bugs in stdout/stderr handling by
[@&#8203;hyangah](https://togithub.com/hyangah) in
[go-delve/delve#3522
- pkg/terminal: support vscode in edit command by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3524
- Use a valid timezone in tested binary by
[@&#8203;upils](https://togithub.com/upils) in
[go-delve/delve#3527
- proc: implement follow exec mode on Windows by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3507
- service/test: disable TestClientServer_chanGoroutines with rr backend
by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3492
- proc/native: use cgo instead of C for freebsd by
[@&#8203;4a6f656c](https://togithub.com/4a6f656c) in
[go-delve/delve#3529
- proc: use DW_AT_trampoline to detect auto-generated code by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3528
- proc: use stack machine to evaluate expressions by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3508
- proc: fix comment typos by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3531
- proc: add min and max builtins by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3530
- CI: update staticcheck version by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3533
- proc: remove expr evaluation goroutine from EvalExpressionWithCalls by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3532
- service/api: use bitfield for prettyprint flags by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3536
- proc: allow evaluator to reference previous frames by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3534
- \*: Add explicit code of conduct by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3540
- proc,service/dap,proc/gdbserial: fixes for debugserver
--unmask-signals by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3541
- \*: release 1.21.2 (for master) by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3546
- proc/native: wherever we check for exited we should also check
detached by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3547
- pkg/proc: add inline function support for stripped binaries by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3549
- cmd: fix a bunch of linter warnings from GoLand by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3550
- proc: add regression test for issue
[#&#8203;3548](https://togithub.com/go-delve/delve/issues/3548) by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3553
- \*: update dependencies by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3552
- service: fix a bunch of linter warnings from GoLand by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3551
- \*: Correct spelling mistakes by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3555
- pkg,service: Remove redundant build constraints by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3556
- Shorten variable types by
[@&#8203;stefanhaller](https://togithub.com/stefanhaller) in
[go-delve/delve#3535
- TeamCity: reupgrade linux/386 builder to Go 1.21 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3560
- pkg/proc: improve support unwinding from sigpanic by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3559
- pkg/proc: unskip passing tests and reorganize by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3561
- chore: use strings.Contains instead by
[@&#8203;testwill](https://togithub.com/testwill) in
[go-delve/delve#3562
- pkg,service: remove unnecessary convertions by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3564
- \*: remove checks for TRAVIS env variable by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3568
- terminal: clear substitute path rules cache when config is used by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3567
- service: fix typo in variable name by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3575
- TeamCity: remove windows/arm64 builders from chain by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3572
- proc: simplify and generalize runtime.mallocgc workaround by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3571
- proc/gdbserial: refactor parsing of key-value pairs from gdb protocol
by [@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3574
- Extract tip builds into a separate sub-project by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3578
- pkg,service/dap: use switch instead of if-else-if by
[@&#8203;alexandear](https://togithub.com/alexandear) in
[go-delve/delve#3576
- CI: update teamcity settings by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3579
- pkg/proc: use gore to obtain info from stripped binaries by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3577
- add missing import in TeamCity DSL by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3580
- update TeamCity DSL version to 2023.05 and remove tip configurations
from Aggregator by [@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3581
- proc: fix TestIssue1101 flake by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3585
- proc: skip trapthread for harcoded breakpoints after manual stop by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3582
- tests: fix tests in go1.22 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3583
- all: run go mod tidy by
[@&#8203;prattmic](https://togithub.com/prattmic) in
[go-delve/delve#3589
- add all branches but PRs to filter by
[@&#8203;artspb](https://togithub.com/artspb) in
[go-delve/delve#3590
- service/dap: fix close on closed channel panic by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3573
- Show pprof labels in thread names by
[@&#8203;stefanhaller](https://togithub.com/stefanhaller) in
[go-delve/delve#3501
- \*: Use forked goretk/gore module by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3597
- proc: make some type casts less counterintuitive by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3596
- teamcity,version: add 1.22 to supported versions and CI matrix by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3601
- proc: fix ppc64 arch name check by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3608
- goversion: include pre-releases in VersionAfterOrEqual check by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3607
- service/dap: fix close on closed channel by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3609
- proc: fix TestPackageRenames on go1.22 on linux/386 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3610
- \*: Update gore dep for 1.22 by
[@&#8203;derekparker](https://togithub.com/derekparker) in
[go-delve/delve#3611
- \*: release version 1.22.0 by
[@&#8203;aarzilli](https://togithub.com/aarzilli) in
[go-delve/delve#3606

#### New Contributors

- [@&#8203;upils](https://togithub.com/upils) made their first
contribution in
[go-delve/delve#3527
- [@&#8203;testwill](https://togithub.com/testwill) made their first
contribution in
[go-delve/delve#3562
- [@&#8203;prattmic](https://togithub.com/prattmic) made their first
contribution in
[go-delve/delve#3589

**Full Changelog**:
go-delve/delve@v1.21.1...v1.22.0
**Curated Changelog**:
https://github.com/go-delve/delve/blob/master/CHANGELOG.md#1220-2023-12-29

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/parca-dev/parca-agent).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
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.

2 participants