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

Support positional parameters as integer literals #1982

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

viktormalik
Copy link
Contributor

@viktormalik viktormalik commented Aug 24, 2021

In all places where an integer literal is expected, we now also accept an integer positional parameter.
These are namely:

  • builtin function calls (lhist, buf, signal, strncmp)
  • array indices.

Fixes #1980.

Note: the only function that is not supported is str, since the solution conflicts with the way str currently handles positional parameters.

Checklist
  • Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

src/bpftrace.cpp Outdated
}
}

LOG(ERROR) << "Expected integer literal, got " << expr->type;
Copy link
Contributor

@fbs fbs Aug 25, 2021

Choose a reason for hiding this comment

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

I think we should drop this error, i see every user of this already has its own error handling. Adding an extra error without location on top of that doesn't sound helpful.

Or we could return the error too and append that to the error message, that would be useful but somewhat tricky to do. Std::variant works for it but quite verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with dropping the error, it doesn't really say much.

Returning the error and appending it to the message could be useful for the other error in this function (line 2215) as it always prints to the default stderr even if the function is called from semantic analyser (which normally prints to its own err_ stream). The question is whether it is worth the effort.

In all places where an integer literal is expected, accept also an
integer positional parameter.
These are namely:
- builtin function calls (lhist, buf, signal, strncmp)
- array indices

Adds unit and runtime tests for this.

The only function that is not supported is "str", since the solution
conflicts with the way "str" currently handles positional parameters.
@fbs fbs merged commit cf9dbf2 into bpftrace:master Sep 10, 2021
@viktormalik viktormalik deleted the posparam-calls branch September 21, 2021 06:07
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.

strncmp length parameter uses positional parameter's index instead of its value
2 participants