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

Fix calling ntop on tracepoint args #1365

Merged
merged 2 commits into from
Jun 5, 2020
Merged

Conversation

danobi
Copy link
Member

@danobi danobi commented Jun 1, 2020

Commit f05b4cd ("Introduce Type::ctx") introduced a ctx type
for values that come from the bpf prog context. This subtly broke ntop()
because ntop was not yet aware of Type::ctx.

Fix this by relaxing semantic analyser check to include Type::ctx.
Similarly relax for codegen.

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

danobi added 2 commits June 1, 2020 14:37
Commit f05b4cd ("Introduce Type::ctx") introduced a `ctx` type
for values that come from the bpf prog context. This subtly broke ntop()
because ntop was not yet aware of Type::ctx.

Fix this by relaxing semantic analyser check to include Type::ctx.
Similarly relax for codegen.
@danobi
Copy link
Member Author

danobi commented Jun 1, 2020

I wonder if Type::ctx should be limited to just the ctx builtin. Reasoning is that a value coming from the prog context is more an attribute and not a type unto itself. For example, saddr_v6 can be both an array and from the prog ctx. It doesn't necessarily have to be one or the other.

@mmisono any thoughts?

@fbs
Copy link
Member

fbs commented Jun 1, 2020

I wonder if Type::ctx should be limited to just the ctx builtin. Reasoning is that a value coming from the prog context is more an attribute and not a type unto itself.

I'm working on replacing cast with a pointer and a record type. As ctx can currently be both and more which makes it confusing I'm going to remove ctx too and make it an attribute instead.

https://github.com/fbs/bpftrace/tree/pointer_type

@danobi
Copy link
Member Author

danobi commented Jun 1, 2020

Sounds good. Looking forward to seeing that merged :)

Copy link
Collaborator

@mmisono mmisono left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

I thought Type::ctx simplified the implementation but actually there are several corner cases and I've thought some rework is needed. Making ctx an attribute seems good.

@danobi danobi merged commit b6f226d into bpftrace:master Jun 5, 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.

3 participants