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.

Sign up for GitHub

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

Jump to bottom

Support patternized module name for dynamic patch #1211

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support patternized module name for dynamic patch #1211

wants to merge 6 commits into from

Conversation

ParkHanbum
Copy link
Contributor

@ParkHanbum ParkHanbum commented Oct 18, 2020 •
edited
Loading

uftrace provides pattern matching on function names However, that is not supported for module names.
so, I added pattern matching for module names.

here is use case against testcase 224 dynamic_lib

ORIGIN

uftrace live --no-pager --no-event -L/home/m/git/uftrace -Pmain [email protected] [email protected] --no-libcall t-dynmain
# DURATION     TID     FUNCTION
            [ 20477] | main() {
            [ 20477] |   lib_a() {
            [ 20477] |     lib_b() {
   2.900 us [ 20477] |       lib_c();
   4.600 us [ 20477] |     } /* lib_b */
   5.800 us [ 20477] |   } /* lib_a */
            [ 20477] |   lib_d() {
            [ 20477] |     lib_e() {
   1.300 us [ 20477] |       lib_f();
   2.200 us [ 20477] |     } /* lib_e */
   3.000 us [ 20477] |   } /* lib_d */
  22.400 us [ 20477] | } /* main */

CHANGED

uftrace live --no-pager --no-event -L/home/m/git/uftrace_my -Pmain -P.@libdyn* --no-libcall t-dynmain
# DURATION     TID     FUNCTION
            [ 19085] | main() {
            [ 19085] |   lib_a() {
            [ 19085] |     lib_b() {
   2.500 us [ 19085] |       lib_c();
   4.600 us [ 19085] |     } /* lib_b */
   6.300 us [ 19085] |   } /* lib_a */
            [ 19085] |   lib_d() {
            [ 19085] |     lib_e() {
   1.400 us [ 19085] |       lib_f();
   2.600 us [ 19085] |     } /* lib_e */
   3.600 us [ 19085] |   } /* lib_d */
  22.600 us [ 19085] | } /* main */

@ParkHanbum
dynamic: add new struct module_patt_list
7125ee7 
a new struct 'module_patt_list' has been added to support pattern
matching on module names. this structure divides the list of function
name patterns by module and stores them in a list format.

Signed-off-by: Hanbum Park <[email protected]>
@ParkHanbum
dynamic: rename patt_list to func_patt_list for clearity
6dbb40d 
prior to pattern support for module names, patt_list was used
simultaneous to match module names and function names. after pattern
support for module names patt_list will only be used for function name
matching, so we have renamed it to clarify this.

Signed-off-by: Hanbum Park <[email protected]>
@ParkHanbum
dynamic: add functions to support the pattern for module name
72ffe27 
functions have been added to support the pattern support for module names.

Signed-off-by: Hanbum Park <[email protected]>
@ParkHanbum
dynamic: Add skip libraries as array
3a10f0a 
libraries that affect the operation of Uftrace should be excluded from
the dynamic code patch list. added list of affected libraries.

Signed-off-by: Hanbum Park <[email protected]>
@ParkHanbum
dynamic: Apply pattern matching on module names when dynamic patching
718183e 
the changed logic is as follows:

when creating a list to be dynamically patched,
-. creates a list of functions to patch by module by separating the
specified dynamic patch targets.

when performing dynamic patching for each module,
-. if the name of the currently searched module exists in skip_libs, it
moves to the next module.
-. search the module list and check whether the current target module
name is a dynamic patch target.
-. searching the symbol of the module registered in the function list of
the module list. perform dynamic patching if find matched symbol.

Signed-off-by: Hanbum Park <[email protected]>
@ParkHanbum
dynamic: Releases resources of module list
d36861c 
since the module list is additionally allocated to support pattern
matching for the module name, we added logic to release it.

Signed-off-by: Hanbum Park <[email protected]>
@honggyukim
Copy link
Collaborator

Hi @ParkHanbum, it'd be better if you post the way how we can reproduce the difference with a simple example using "Before" and "After".

@ParkHanbum
Copy link
Contributor Author

@honggyukim I should have done it, thank you for telling me. it was updated at first comment.

namhyung
namhyung requested changes Nov 17, 2020
View reviewed changes
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

I think we can simply change type of patt_list.module to struct uftrace_pattern, no?

libmcount/dynamic.c
struct uftrace_pattern module_patt;
char *module;
};

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this should be a separate commit. Please merge this into where it's actually used.

libmcount/dynamic.c
free(pl);
list_del(&ml->list);
free(ml->module);
free(ml);
Copy link
Owner

Choose a reason for hiding this comment

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

In general, I think it's better to put the release code along with the allocation code. So we can easily find where they are and make sure not to create any leaks.

Also please consider adding an unit test for new functionalities. It's also good to check memory errors with ASAN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namhyung namhyung namhyung requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ParkHanbum @honggyukim @namhyung