Remove the --btf option #1669
Merged
Remove the --btf option #1669
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viktormalik
force-pushed
the
remove-btf-option-new
branch
from
7c86bed
to
b83ec62
Compare
|
Skimmed the code and it looks reasonable. I'll spend some time tomorrow to try and break it. |
|
Can you write up the set of "rules" regarding how type definitions are chosen/acquired? Would help users understand errors and help us maintain behavior over time. Also, there should be more runtime tests for this PR to test combinations of user-defined and BTF defined types. We can use |
|
Seeing some errors with dcsnoop.bt: |
Good idea. Actually, there are rules from the user point of view (i.e., what happens if I redefine a kernel type?) and from the implementation point of view (i.e., how the types are actually resolved?). I'd put the former into the reference guide and the latter as a comment into the code, sounds reasonable?
Ok, I'll try to come with some tests. |
Sounds great |
There are 3 main refactorings done: - make input, args, and input_files to be class members (instead of local variables of the parse() method) so that they don't have to be passed around as function arguments, - factor out resolution of incomplete and of unknown types into separate methods, - factor out parsing of files and subsequent error handling and diagnostics into a new method ClangParserHandler::parse_file.
BTF is used automatically, if it is available and the user did not define any types (via user definitions or includes) that redefine types from BTF. If the user defined aby such types, BTF is not parsed (and the user must specify all the neccessary types). Checking if the user redefined any types is done by running Clang parser and searching the produced error messages for the "redefinition" word. The --btf (-b) CLI option is left as no-op for compatibility.
viktormalik
force-pushed
the
remove-btf-option-new
branch
from
b83ec62
to
cf2a6ab
Compare
danobi
reviewed
Feb 3, 2021
docs/reference_guide.md
Outdated
| @@ -108,6 +108,7 @@ discussion to other files in /docs, the /tools/\*\_examples.txt files, or blog p | |||
| - [1. `printf()`: Per-Event Output](#1-printf-per-event-output) | |||
| - [2. `interval`: Interval Output](#2-interval-interval-output) | |||
| - [3. `hist()`, `printf()`: Histogram Printing](#3-hist-print-histogram-printing) | |||
| - [BTF Types](#btf-types) | |||
There was a problem hiding this comment.
Maybe this is a bit more descriptive
Suggested change
| - [BTF Types](#btf-types) | |
| - [BTF Support](#btf-types) |
There was a problem hiding this comment.
Right, I fixed it.
viktormalik
force-pushed
the
remove-btf-option-new
branch
from
cf2a6ab
to
8d17efa
Compare
danobi
approved these changes
Feb 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an improved version of #1506, which caused type redefinition conflicts for some cases. Now, all the examples mentioned in the old PR should work.
BTF is parsed automatically if it is available and if the user didn't define any type (either via user definitions or via includes) that would redefine some type from BTF.
I think that this also closes #1588. It would seem nicer to allow the user to redefine a part of the types and take the rest from BTF, but I believe that could cause some unexpected behaviour (see this comment for details).
The only problem of this implementation is that it checks for redefinitions by running the Clang parser and then checking the error messages. This is not a nice solution, but I haven't been able to find a better one, yet. If you have any ideas, please bring them in.
Also, I'm not sure if this solves the problem from #1511, as
linux/types.hwill be used when BTF is not and that may cause redefinition errors withsys/types.h. It shouldn't be hard to resolve this in a way similar to this PR, but I haven't been able to come with a reproducer.Checklist
docs/reference_guide.mdCHANGELOG.md