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 type resolution for field access via variables #1450

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

viktormalik
Copy link
Contributor

@viktormalik viktormalik commented Aug 4, 2020

Fixes #1362

The problem is that the field analyser was not able to correctly resolve struct field types if a variable (of struct type) was used because it doesn't remember the variable type.

It only worked for cases when the variable was used right after its definition since the type was remembered in the type_ field of the field analyser (see the linked issue for examples).

This PR fixes the problem by storing the variable type and using it when the variable is used.

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

@fbs
Copy link
Member

fbs commented Aug 4, 2020

Would it be possible to test this behaviour? Our ci doesn't run these kfunc tests yet but they would be good to have to avoid regressions

@viktormalik
Copy link
Contributor Author

Would it be possible to test this behaviour? Our ci doesn't run these kfunc tests yet but they would be good to have to avoid regressions

Good point, I added a new test case for this. Also verified that the test doesn't pass without this fix.

@viktormalik
Copy link
Contributor Author

One of the CI tests failed but it is not related to this PR. I think that a restart should fix that.

Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

Awesome

@danobi
Copy link
Member

danobi commented Aug 7, 2020

Rerunning tests

@viktormalik
Copy link
Contributor Author

Failed on Docker container build this time.

@fbs
Copy link
Member

fbs commented Aug 10, 2020

yeey tests finally pass. Can you fix the clang formatting issues? :)

@danobi
Copy link
Member

danobi commented Aug 11, 2020

@viktormalik can you update the changelog too?

If a variable of a (pointer to) struct type is defined, its type must be
saved. Later, when the variable is used, the stored type helps to
correctly resolve types of following field accesses.

Fixes iovisor#1362.

Add a unit test that requires this fix.
@viktormalik
Copy link
Contributor Author

@viktormalik can you update the changelog too?

Sure, done.

@fbs fbs merged commit 62a8cf5 into bpftrace:master Aug 12, 2020
@viktormalik viktormalik deleted the field-analyser-fix branch August 18, 2020 06:19
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.

Unable to resolve struct in struct using BTF in some cases
3 participants