-
Notifications
You must be signed in to change notification settings - Fork 349
trace: Add Kconfig options to enable verbose tracing only from certain classes #2133
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
Sorry, but I'm actually not in favour of this change (and others like this throughout the patch. The reasons are very simple: (1) it makes it more difficult to grep for these symbols, e.g. if you want to find all uses of
TRACE_CLASS_SRCyou won't find this any more. Also it makes following symbols in editors more difficult. (2) it makes it look different from the surrounding lines, e.g. the line above, which usesTRACE_CLASS_SRCin full.If (1) is a trade-off - you win code compactness but you lose clarity, I think (2) is really not good - either convert all of them, or leave them all as is, don't mix them.
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.
Using "TRACE_CLASS_SRC" directly will not work correctly as that will be expanded to a number and will break the very idea of this PR, to disable verbose traces for particular classes. Maybe TRACE_CLASS_SRC with some prefix or suffix?
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.
@lyakh It was already picked up by @paulstelian97 and explained on other occurence of this change, here: #2133 (comment)
The intermediate macro
trace_event_comp, causes prematureTRACE_CLASS_*token expansion, which could be solved by quite some dirty work under the hood intrace_eventmacro family expansions, or (in my opinion) more elegantly, here.One could argue that greping for
TRACE_CLASS_*doesn't make much sense anymore, as we've already wrapped (almost, reaching 100% soon) all traces in their respectivetrace_*macros.If you have an idea how to solve token pasting and expansion more elegantly, I'm open for suggestions, but I honestly think that this route is cleaner than appending
_to tokens to prevent their expansion, and then additionally accounting for it intrace-verbose-private.h.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.
@paulstelian97 thanks for the explanation, I see now. But now my question is then: is it really worth it? Is it really worth adding all those levels of macros if we anyway have the trace-verbose-private.h header, which has a bunch of defines for each of the
CONFIG_TRACE_VERBOSE_*options? Wouldn't it be much easier and compact to just addin every affected file? Most of them would then just involve changing a couple of lines at the top, others, calling generic macros directly from the code, like dai.c, without wrapping them, will need such a define added at the top, which would (arguably) also simplify them?
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.
@lyakh Tbh if we're doing that I'd say having the definition for
trace_asrc_with_idsand every other such macro in a common header and make those macros public would be an easy way out. That sure is a large redo of this change and everyone would thus have to use these custom functions instead of the raw trace_event functions to benefit from that disabling.Also in case we go with that, use the UNUSED macro instead of a do-while-0 loop. It was built specifically for using in disabled traces.
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.
@paulstelian97 I'd be fine keeping those defines in respective .c as long as they're used only there. I don't see a need for a common header if all or at least most of them are only used in a single file. However, if sufficiently many of them are or can be used in multiple files, then yes, we could use a common header.