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

trace.py support for args with Go's gc compiler #934

Open
brendangregg opened this issue Feb 2, 2017 · 10 comments
Open

trace.py support for args with Go's gc compiler #934

brendangregg opened this issue Feb 2, 2017 · 10 comments

Comments

@brendangregg
Copy link
Member

brendangregg commented Feb 2, 2017

trace's arg# aliases work with Go when compiled with gccgo ("gccgo -o hello hello.go"). But Go's gc compiler ("go build hello.go") uses a different function calling convention, based on the Plan 9 compiler.

Is it important to fix this? Does everyone run gccgo binaries (where trace already works), because they are expected to be faster, and not Go's compiler? I don't know. I've added a prio:low tag for now.

I wrote some quick docs on the issues here: http://www.brendangregg.com/blog/2017-01-31/golang-bcc-bpf-function-tracing.html

And included a proof of concept:

# trace '/home/bgregg/functions:main.add "%d %d" go1, go2'
PID    TID    COMM         FUNC             -
17555  17555  functions    main.add         42 13

The change I made was a dirty hack, and needs to be rewritten properly. I'd done this:

# diff -u trace.py trace-go.py 
--- trace.py	2017-02-01 19:34:01.477727396 +0000
+++ trace-go.py	2017-02-02 09:28:23.796606244 +0000
@@ -209,6 +209,8 @@
 
         aliases = {
                 "retval": "PT_REGS_RC(ctx)",
+                "go1": "1;bpf_probe_read(&__data.v0,sizeof(__data.v0),(void *)ctx->sp+8)",
+                "go2": "1;bpf_probe_read(&__data.v1,sizeof(__data.v1),(void *)ctx->sp+16)",
                 "arg1": "PT_REGS_PARM1(ctx)",
                 "arg2": "PT_REGS_PARM2(ctx)",
                 "arg3": "PT_REGS_PARM3(ctx)",

They should be "goarg1", "goarg1", etc. I suspect this also needs to be done differently so that they can be used in tests, eg, "(goarg1 == 42)". And this should use macros instead of ctx->sp.

I'd also check the following resources to confirm that this approach is valid:

@drzaeus77
Copy link
Collaborator

If you're looking for datapoints, I used to (go 1.0) rely on gccgo when I needed to link in external C libraries, but nowadays native go is my go-to.

@goldshtn
Copy link
Collaborator

goldshtn commented Feb 3, 2017

I'd go for a helper syntax like $ptr(x) which would do the bpf_probe_read automatically, and then the goargN aliases can be made equivalent to $ptr(PT_REGS_SP(ctx)+N*8), at least on 64-bit platforms. But that way we also gain the generic ability to do memory reads, which might prove useful. (E.g. in a retprobe when we know rax points to a value we want, we'd be able to do $ptr(ctx->ax).)

But there's also always the general remark that we keep pushing the trace/argdist syntax further and further, and there has to be a point where we recommend a dedicated script. @brendangregg

@kelleyk
Copy link

kelleyk commented Feb 5, 2017

We also use gc binaries, so I've been doing something similar to this "dirty hack".

I'd be happy to do the work to get this functionality added to the trace script, but I'm not sure exactly what solution you'd be most happy to see. If we go for the $ptr() approach, for example, it seems like we'll need to generalize the paren-matching logic in _parse_probe. Would changes that extensive be welcome?

@brendangregg
Copy link
Member Author

Someone just told me about this, possible for go 1.8:

https://go-review.googlesource.com/#/c/28832/

This is work in progress for passing arguments (parameters?
what is the proper word?) and results in registers.

Maybe Go is going to switch to the x86_84 ABI?

@goldshtn
Copy link
Collaborator

@kelleyk I think having $ptr would be nice in general. All the parsing work is pretty hacky right now, so if you feel like cleaning that up or generalizing, it would be very welcome. With the direction we're currently heading, it looks like we could be adding more helpers like this in the future.

@kelleyk
Copy link

kelleyk commented Feb 13, 2017

I think that the details of the new Go ABI are still up in the air, and from golang/go#18597 it sounds like the change is motivated by performance and not by standardization. There is some mention of making the new ABI a superset of the platform ABI, but also not a lot of hesitation to stray when useful (by, for example, skipping the 160-byte save area on s390x). It also seems like it would land in go1.9 at the earliest, since the go1.8 tree was feature-frozen at the end of October.

@goldshtn Thanks for the guidance! Let me see if I can find a minute to take a stab at $ptr and the supporting parsing improvements, then.

@nyarly
Copy link

nyarly commented Apr 26, 2018

I ran across this and started toying with a PR. What I'm finding is that the changes required to trace.py are more extensive - at least the assignment templating needs to change to accomodate strings or multiple arguments. I'm tempted, therefore, to copy trace.py to gotrace.py and hack that up until it can manage the gc style of parameter handling. Would that be acceptable as a PR?

@goldshtn
Copy link
Collaborator

@nyarly Anything that works would be better than having no solution at all, but I think having a separate tool in the long run would not be great. If you're so inclined, you could hack on gotrace.py temporarily, just to see if the approach works, but I think we should have a single tool that can support both use cases. And if it means a massive refactoring of trace.py's (admittedly already hacky) code, I'm afraid that's just a prerequisite for this work. Merging gotrace.py risks diverging in a way that's detrimental to our goals here, in my opinion.

@nyarly
Copy link

nyarly commented Apr 26, 2018

I agree that separate tooling is sub-optimal. Candidly, I think refactoring trace.py such that it can handle both C and Go is more than I'd want to take on. I'll put together gotrace.py and revisit.

@nyarly nyarly mentioned this issue Apr 30, 2018
@nyarly
Copy link

nyarly commented May 14, 2018

Implemented #1719 as a first cut of this.

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

No branches or pull requests

5 participants