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

Check a call function is available for a probe type #1646

Merged
merged 4 commits into from
Nov 24, 2020

Conversation

mmisono
Copy link
Collaborator

@mmisono mmisono commented Nov 22, 2020

Some call functions (e.g., reg()) is not available for some probe types. To check this, add check_available(call, type) function. This function returns true if the call is available for that type. Also, disable reg() in k(ret)func using this function. Previously that is mistakenly allowed and resulted in invalid BPF codes.

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

@mmisono mmisono mentioned this pull request Nov 22, 2020
2 tasks
@mmisono
Copy link
Collaborator Author

mmisono commented Nov 22, 2020

This is a continuation of #1380. kfunc reg() tests would conflicts #1641.

}
}

if (type == ProbeType::invalid)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this to the top and then remove all the invalid cases from the switch statements? Or does that trigger a missing case warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, -Wswitch emits warnings. warning: enumeration value 'invalid' not handled in switch [-Wswitch]

@mmisono
Copy link
Collaborator Author

mmisono commented Nov 23, 2020

rebased and update tests since #1641 is just merged.

@danobi
Copy link
Member

danobi commented Nov 23, 2020

We can make a call to check_available(..) at the beginning of the function and delete the other callsites right? Would reduce duplicated code (especially the loop).

@fbs
Copy link
Contributor

fbs commented Nov 23, 2020

I wonder if its doable to write a simple spec for each call that handles this and all the other standard check things for calls (and builtins). E.g.

CallSpec {
 .name = "str",
 .probe_types = any,
 .required_features = {somefeature},
 .min_args = 1,
 .max_args = 2,
 .arg_types = [
  [{ type = integer, literal = true}, { type = pointer } ], // arg0
  [{ type = integer }], // arg1
  .func = &semantic_analyser::call_str
]}

It would remove quite a bit of basically duplicate code everywhere, the deep nested long functions and would help standardize error messages. It should also make it easier to get a quick overview of the calls we have an what kind of arguments they take, might even be possible to generate nice help format output from it.

But there probably are some cases that make it hard to do

@@ -810,8 +811,7 @@ void SemanticAnalyser::visit(Call &call)
for (auto &ap : *probe_->attach_points)
{
ProbeType type = probetype(ap->provider);
if (type != ProbeType::usdt && type != ProbeType::uretprobe &&
type != ProbeType::uprobe)
if (!check_available(call, type))
{
LOG(ERROR, call.loc, err_)
<< "uaddr can only be used with u(ret)probes and usdt probes";
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like we now need to keep track of this in two places. We have to update check_available to reflect the probe types and then update the error message too. Can't we update check_available to output the err msg, the other checks do it as well.

Changing the signature to check_available(Call &c, std::vector<probetype &> t) might make that easier too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed so that the error messages are printed out when check_available() failed.
6fcf1a7#diff-042d9701e67f79452452faac3c625c3ca83e7692abd3274956abe30d58ae0c39R459-R467

@mmisono
Copy link
Collaborator Author

mmisono commented Nov 23, 2020

fbs's proposal seems great. For now, I implemented danobi's suggestion.

Some call functions are available for some probe types. This function
checks it.
Previously, checks of reg() availability for kfunc is missing. The
previous commit adds this check. Add the test to ensure for kfunc not to
be able to call reg().
@mmisono
Copy link
Collaborator Author

mmisono commented Nov 24, 2020

rebased

@mmisono mmisono merged commit 1e84062 into bpftrace:master Nov 24, 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.

None yet

3 participants