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

common: include new logging functions in linkers files #6066

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Mar 21, 2024

It also fixes a few issues of call_stacks_analysis.


This change is Reviewable

@grom72 grom72 added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Mar 21, 2024
@grom72 grom72 requested review from janekmi and osalyk March 21, 2024 07:53
Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

🐐

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)


utils/call_stacks_analysis/make_extra.py line 32 at r2 (raw file):

        calls['core_log_va'] = ['core_log_default_function']
        calls['core_log'] = ['core_log_va']
        calls['core_log_set_function'] = ['core_log']

Are you 100% sure cflow does not get it right?

Code quote:

        calls['core_fini'] = ['out_fini', 'core_log_fini', 'last_error_msg_fini']
        calls['common_init'] = ['core_init', 'util_mmap_init']
        calls['common_fini'] = ['util_mmap_fini', 'core_fini']
        calls['Last_errormsg_key_alloc'] = ['_Last_errormsg_key_alloc']
        calls['_Last_errormsg_key_alloc'] = ['os_once', 'os_tls_key_create']
        calls['core_log_va'] = ['core_log_default_function']
        calls['core_log'] = ['core_log_va']
        calls['core_log_set_function'] = ['core_log']

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)

@grom72 grom72 force-pushed the call-stack-analysis-w-log branch 2 times, most recently from ad03b4f to 8361240 Compare March 21, 2024 12:51
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)


utils/call_stacks_analysis/make_extra.py line 32 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Are you 100% sure cflow does not get it right?

No ;)
But it seems that more functions can be removed from here.
Do not ask me "why?"
We can check this section one more time when all PR for 2.1.0 are merged.

@grom72 grom72 force-pushed the call-stack-analysis-w-log branch 3 times, most recently from c04b2c5 to 30ee4a7 Compare March 21, 2024 14:36
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)


utils/call_stacks_analysis/make_extra.py line 32 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

No ;)
But it seems that more functions can be removed from here.
Do not ask me "why?"
We can check this section one more time when all PR for 2.1.0 are merged.

Done.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @grom72)


src/libpmemobj/libpmemobj.link.in line 21 at r6 (raw file):

		pmemobj_log_get_threshold;
		pmemobj_log_set_function;
		pmemobj_log_set_threshold;

Duplicate.


utils/call_stacks_analysis/make_extra.py line 25 at r6 (raw file):

        # common
        calls['core_init'] = ['util_init', 'core_log_init', 'out_init']
        calls['core_fini'] = ['out_fini', 'core_log_fini', 'last_error_msg_fini']
  1. As far as I can tell, all of it is already in the cflow output.
  2. Note common: Clean-up Valgrind usage in the project #5950 leaked a part of this change so you have to rebase.

utils/call_stacks_analysis/make_extra.py line 29 at r6 (raw file):

        calls['common_fini'] = ['util_mmap_fini', 'core_fini']
        calls['Last_errormsg_key_alloc'] = ['_Last_errormsg_key_alloc']
        calls['_Last_errormsg_key_alloc'] = ['os_once', 'os_tls_key_create']

No longer exists.


utils/call_stacks_analysis/make_extra.py line 30 at r6 (raw file):

        calls['Last_errormsg_key_alloc'] = ['_Last_errormsg_key_alloc']
        calls['_Last_errormsg_key_alloc'] = ['os_once', 'os_tls_key_create']
        calls['core_log_init'] = ['core_log_default_init', 'core_log_set_function']

core_log_init is not missing anything.


utils/call_stacks_analysis/make_extra.py line 44 at r6 (raw file):

        return calls

def function_pointers(calls: Calls) -> Calls:

Suggestion:

core_function_pointers

utils/call_stacks_analysis/make_extra.py line 431 at r6 (raw file):

def main():
        extra_calls = inlines({})
        extra_calls = function_pointers(extra_calls)

Suggestion:

core_function_pointers

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
@grom72 grom72 force-pushed the call-stack-analysis-w-log branch 2 times, most recently from 9580689 to bd579dd Compare March 26, 2024 15:16
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r7.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @janekmi)


src/libpmemobj/libpmemobj.link.in line 21 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Duplicate.

Done.


utils/call_stacks_analysis/make_extra.py line 25 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…
  1. As far as I can tell, all of it is already in the cflow output.
  2. Note common: Clean-up Valgrind usage in the project #5950 leaked a part of this change so you have to rebase.

I propose to solve the inline functions problem in a separate PR.
The existing solution works and provides proper results.


utils/call_stacks_analysis/make_extra.py line 29 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No longer exists.

Done.


utils/call_stacks_analysis/make_extra.py line 30 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

core_log_init is not missing anything.

.


utils/call_stacks_analysis/make_extra.py line 44 at r6 (raw file):

        return calls

def function_pointers(calls: Calls) -> Calls:

Done.


utils/call_stacks_analysis/make_extra.py line 431 at r6 (raw file):

def main():
        extra_calls = inlines({})
        extra_calls = function_pointers(extra_calls)

Done.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (d4bd6f2) to head (dd0b61b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6066      +/-   ##
==========================================
+ Coverage   70.12%   70.13%   +0.01%     
==========================================
  Files         133      133              
  Lines       19568    19568              
  Branches     3261     3261              
==========================================
+ Hits        13722    13724       +2     
+ Misses       5846     5844       -2     

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @janekmi)

@grom72 grom72 added this to the 2.1.1 milestone Mar 27, 2024
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @janekmi)


utils/call_stacks_analysis/make_extra.py line 25 at r6 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

I propose to solve the inline functions problem in a separate PR.
The existing solution works and provides proper results.

Done.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72)


src/libpmemobj/libpmemobj.link.in line 2 at r9 (raw file):

# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2014-2024, Intel Corporation

It seems this file is not under the license check. Funny.


utils/call_stacks_analysis/make_extra.py line 427 at r9 (raw file):

        return list(set(callees))

# XXX to be redesign

Please add pmem/pmdk/issues number.


utils/call_stacks_analysis/make_extra.py line 430 at r9 (raw file):

# It seems that cflow properly identifies inline functions but in some
# situations does not pars properly funtions that are used inside inline
# funcion:

Suggestion:

# situations does not parse properly functions that are used inside inline
# functions:

utils/call_stacks_analysis/make_extra.py line 437 at r9 (raw file):

#                       core_log_init()
#                       out_init()
#               util_mmap_init()

Drop a little bit of unnecessary info.

Suggestion:

# libpmem_init() <__attribute__ ((constructor)) void libpmem_init (void) at .../src/libpmem/libpmem.c:23>:
#       common_init() <inline void common_init (...) at .../src/common/pmemcommon.h:19>:
#               core_init() <inline void core_init (...) at .../src/core/pmemcore.h:24>:
#                       util_init()
#                       core_log_init()
#                       out_init()
#               util_mmap_init()

utils/call_stacks_analysis/make_extra.py line 444 at r9 (raw file):

# Acccording to very brife analysis all calles listed on the right hand side
# shall be added to extra_entry_points.txt but they are not required in
# extra_calls.json file

Suggestion:

# It seems all callees of inline function (in the example above: util_init(),
# core_log_init(), out_init()) are not further processed. This causes
# the callees of these functions to be reported as not called (an error reported
# by the make_call_stacks.py script).
# Adding callees of inline functions as entry points allows forcing cflow to
# include these missing calls. Not in their natural place but as artificial
# entry points.
#
# cflow fails to continue:
# function A -> inline function -> inline function's callee -> STOP
#
# forced entry point:
# inline function's callee -> ...

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @janekmi)


utils/call_stacks_analysis/make_extra.py line 427 at r9 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please add pmem/pmdk/issues number.

Done.


utils/call_stacks_analysis/make_extra.py line 430 at r9 (raw file):

# It seems that cflow properly identifies inline functions but in some
# situations does not pars properly funtions that are used inside inline
# funcion:

Done.


utils/call_stacks_analysis/make_extra.py line 437 at r9 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Drop a little bit of unnecessary info.

Done


utils/call_stacks_analysis/make_extra.py line 444 at r9 (raw file):

# Acccording to very brife analysis all calles listed on the right hand side
# shall be added to extra_entry_points.txt but they are not required in
# extra_calls.json file

Done.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r11.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@janekmi janekmi merged commit ff32ce0 into pmem:master Mar 27, 2024
9 checks passed
@grom72 grom72 modified the milestones: 2.1.1, 2.1.0 Mar 28, 2024
@grom72 grom72 deleted the call-stack-analysis-w-log branch May 15, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request Priority: 1 urgent sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants