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

Add RelativeOffset field to UprobeOptions struct #682

Merged
merged 9 commits into from
Jun 1, 2022

Conversation

mertyildiran
Copy link
Contributor

@mertyildiran mertyildiran commented May 24, 2022

Alternative to #683

This PR, for a bpftrace program like below:

#!/usr/bin/env bpftrace

uprobe:./main:"main.probeMe"+42
{
    printf("rax: %d\n", reg("ax"));
}

enables probing without the need of recalculating the offset of main.probeMe (which is already calculated and stored inside offsets field), adding 0x2a and setting Offset field to the resulting value:

up, err := ex.Uprobe("main.probeMe", objs.ExampleUprobe, &link.UprobeOptions{RelativeOffset: 0x2a})
if err != nil {
	log.Fatalf("creating uprobe: %s", err)
}
defer up.Close()

The PR also improves some of the comments because it's hard to understand at first glance that with the block below, the Offset field nullifies the offset that's calculated from the given symbol and the Offset is actually an absolute offset instead of a relative one:

ebpf/link/uprobe.go

Lines 255 to 262 in 951bb28

offset := opts.Offset
if offset == 0 {
off, err := ex.offset(symbol)
if err != nil {
return nil, err
}
offset = off
}

link/uprobe.go Outdated
@@ -49,9 +49,13 @@ type Executable struct {
// UprobeOptions defines additional parameters that will be used
// when loading Uprobes.
type UprobeOptions struct {
// Symbol offset. Must be provided in case of external symbols (shared libs).
// Symbol offset (absolute). Must be provided in case of external symbols (shared libs).
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be the offset from the start (so maybe absolute is not the best term). Maybe leave this comment as is and specify in the comment below that RelativeOffset is an additional offset relative to the calculated or provided offset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

link/uprobe.go Show resolved Hide resolved
link/uprobe.go Outdated
@@ -254,11 +262,12 @@ func (ex *Executable) uprobe(symbol string, prog *ebpf.Program, opts *UprobeOpti

offset := opts.Offset
if offset == 0 {
offset = opts.RelativeOffset
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what about doing offset: offset+opts.RelativeOffset in probeArgs on line 287? this way the "relative offset" is added both for a specified one and for an internally calculated one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Hi, can you add a test that covers the feature?

@mertyildiran
Copy link
Contributor Author

mertyildiran commented May 28, 2022

@lmb I have added a test case with mertyildiran@89cb6f2 But IMHO, exporting the Offset method was a simpler change. Which I have provided as an alternative in the PR description: #683 I'm not sure why the offset method is preferred as unexported:

func (ex *Executable) offset(symbol string) (uint64, error) {

@mertyildiran mertyildiran requested a review from lmb May 28, 2022 18:53
Also change the behaviour to always add RelativeOffset, regardless of
where we got the absolute offset from.
@lmb
Copy link
Collaborator

lmb commented May 31, 2022

Looks good to me, I've pushed 401e03f with small changes:

  • Merge offset and offsetWithOpts, don't allow passing nil opts
  • Add ReferenceOffset even when UprobeOptions.Offset is given. This makes more sense to me since Uprobe.Offset represents the start of the function / symbol. It also means that the behaviour of ReferenceOffset is easier to understand: it is always added to the "base", no matter what.

Regarding the API: RelativeOffset is more convenient for the user than Offset() since they don't have to deal with error handling. So if specifying a relative offset is at least somewhat common going for the more convenient API is better.

If you are OK with these changes I'm happy to merge the PR.

@lmb lmb merged commit 2050a9f into cilium:master Jun 1, 2022
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