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

Allow positional parameters in attachpoint definitions #1328

Merged
merged 7 commits into from
May 20, 2020

Conversation

danobi
Copy link
Member

@danobi danobi commented May 13, 2020

This commit supports using positional parameters as follows:

  # bpftrace -e 'kprobe:$1 { 1 }' -- f
  Attaching 1 probe...

  # bpftrace -e 'kprobe:$1asdf { 1 }' -- f
  stdin:1:1-14: ERROR: Found trailing text 'asdf' in positional parameter index. Try quoting the trailing text.
  kprobe:$1asdf { 1 }
  ~~~~~~~~~~~~~

  # bpftrace -e 'kprobe:$1"asdf" { 1 }' -- f
  Attaching 1 probe...
  cannot attach kprobe, probe entry may not exist
  Error attaching probe: 'kprobe:fasdf'

This closes #606 .

Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md

@mmisono
Copy link
Collaborator

mmisono commented May 17, 2020

This is definitely useful; I found that the followings do not work.

  1. Use as int
% ./src/bpftrace -e 'i:ms:$1 { printf("hi\n"); }' 100 
stdin:1:1-8: ERROR: Failed to parse '': stoi
i:ss:$1 { printf("hi\n"); }
~~~~~~~
  1. Use as offset
sudo ./src/bpftrace -e 'k:vfs_open+$1 { printf("hi\n"); }' 5
stdin:1:1-14: ERROR: Invalid offset
k:vfs_open+$1 { printf("hi\n"); }
~~~~~~~~~~~~~
% sudo ./src/bpftrace -e 'BEG$1 { printf("hi\n"); }' IN
stdin:1:1-6: ERROR: Unrecognized probe type: BEG
BEG$1 { printf("hi\n"); }
~~~~~
% sudo ./src/bpftrace -e 'BEGIN$1 { printf("hi\n"); }' IN
stdin:1:1-8: ERROR: Unrecognized probe type: BEGININ
BEGIN$1 { printf("hi\n"); }
~~~~~~~

@danobi
Copy link
Member Author

danobi commented May 18, 2020

@mmisono I fixed 1-3 with:

commit d925f154416d28d5a5bf8acb9a9748702db1c796 (HEAD -> params_in_aps)
Author: Daniel Xu <dxu@dxuuu.xyz>
Date:   Mon May 18 14:27:39 2020 -0700

    Load positional parameters before driver runs

    Before, we were loading positional parameters into bpftrace after the
    driver ran and before the AST was processed. This was fine for
    positional parameters used inside the probe body but not ok for
    positional parameters parsed in the driver (ie attach point defns).

diff --git a/src/main.cpp b/src/main.cpp
index cafe0b4..d497585 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -346,6 +346,13 @@ int main(int argc, char *argv[])
   BPFtrace bpftrace(std::move(output));
   Driver driver(bpftrace);

+  // positional parameters
+  while (optind < argc)
+  {
+    bpftrace.add_param(argv[optind]);
+    optind++;
+  }
+
   bpftrace.usdt_file_activation_ = usdt_file_activation;
   bpftrace.safe_mode_ = safe_mode;
   bpftrace.force_btf_ = force_btf;
@@ -453,12 +460,6 @@ int main(int argc, char *argv[])

   cap_memory_limits();

-  // positional parameters
-  while (optind < argc) {
-    bpftrace.add_param(argv[optind]);
-    optind++;
-  }
-
   // defaults
   bpftrace.join_argnum_ = 16;
   bpftrace.join_argsize_ = 1024;

What is the issue with 4? That seems like reasonable behavior to me.

@mmisono
Copy link
Collaborator

mmisono commented May 18, 2020

@danobi
oops, there is no issue with 4.

@danobi
Copy link
Member Author

danobi commented May 18, 2020

Ah I figured out why my tests passed initially. driver.parse() runs twice. My tests only had string arguments and the AttachPointParser accepts an empty string as an argument, but cannot accept an empty string as an numeric parameter (stoi fails). But before second driver.parse() runs the positional params are added.

I think the fix I put above is correct (modulo a small change).

@danobi danobi force-pushed the params_in_aps branch 2 times, most recently from 35a6e15 to 730303e Compare May 18, 2020 22:57
docs/reference_guide.md Outdated Show resolved Hide resolved
Non functional change.

This commit moves and renames split_attachpoint(). In the next commit we
will need to access member variables and print out errors.

Also rename to lex_attachpoint because that's actually what it's doing.
This commit supports using positional parameters as follows:

      # bpftrace -e 'kprobe:$1 { 1 }' -- f
      Attaching 1 probe...

      # bpftrace -e 'kprobe:$1asdf { 1 }' -- f
      stdin:1:1-14: ERROR: Found trailing text 'asdf' in positional parameter index. Try quoting the trailing text.
      kprobe:$1asdf { 1 }
      ~~~~~~~~~~~~~

      # bpftrace -e 'kprobe:$1"asdf" { 1 }' -- f
      Attaching 1 probe...
      cannot attach kprobe, probe entry may not exist
      Error attaching probe: 'kprobe:fasdf'

This closes bpftrace#606 .
Before, we were loading positional parameters into bpftrace after the
driver ran and before the AST was processed. This was fine for
positional parameters used inside the probe body but not ok for
positional parameters parsed in the driver (ie attach point defns).
@danobi danobi merged commit 7cc0106 into bpftrace:master May 20, 2020
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.

Positional parameters don't work in probes
2 participants