-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
} | ||
} | ||
|
||
if (type == ProbeType::invalid) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]
cc83318
to
c199250
Compare
rebased and update tests since #1641 is just merged. |
We can make a call to |
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.
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 |
src/ast/semantic_analyser.cpp
Outdated
@@ -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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
fbs's proposal seems great. For now, I implemented danobi's suggestion. |
6fcf1a7
to
04115e7
Compare
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().
04115e7
to
ca5a358
Compare
rebased |
Some call functions (e.g.,
reg()
) is not available for some probe types. To check this, addcheck_available(call, type)
function. This function returns true if the call is available for that type. Also, disablereg()
in k(ret)func using this function. Previously that is mistakenly allowed and resulted in invalid BPF codes.Checklist
docs/reference_guide.md
CHANGELOG.md