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

Stack usage generation enabling. #5887

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Oct 24, 2023

Stack usage generated with every build:
DEFAULT_CFLAGS += -fstack-usage

Stack usage is combined into one file with stack-usage-stat.sh script.

Signed-off-by: Tomasz Gromadzki tomasz.gromadzki@intel.com


This change is Reviewable

@grom72 grom72 added libpmemobj src/libpmemobj DAOS DAOS-related no changelog Add to skip the changelog check on your pull request labels Oct 24, 2023
@grom72 grom72 requested review from janekmi and osalyk October 24, 2023 04:49
@grom72 grom72 added the sprint goal This pull request is part of the ongoing sprint label Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #5887 (a08e391) into master (0dec972) will increase coverage by 2.23%.
The diff coverage is n/a.

❗ Current head a08e391 differs from pull request most recent head ddb2081. Consider uploading reports for the commit ddb2081 to get more accurate results

@@            Coverage Diff             @@
##           master    #5887      +/-   ##
==========================================
+ Coverage   68.77%   71.00%   +2.23%     
==========================================
  Files         131      131              
  Lines       19674    19172     -502     
  Branches     3259     3192      -67     
==========================================
+ Hits        13530    13614      +84     
+ Misses       6144     5558     -586     

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 2 of 2 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)


utils/stack-usage-stat.sh line 17 at r2 (raw file):

    gawk '{print $2 " "  $1 " " $3 " "  $4 " "  $5}' | \
    sort -n -r > src/stat/stack-usage-nondebug.txt

If you want delete line number from log you can use:
sed -i 's/:[0-9]*:[0-9]//g' src/stat/stack-usage-[non]debug.txt
Before: 16448 src/nondebug/libpmemobj/obj.su:obj.c:549:1:obj_vg_check_no_undef static
After: 16464 src/nondebug/libpmemobj/obj.su:obj.c:obj_vg_check_no_undef static

@grom72 grom72 force-pushed the fstack-usage-enable branch from 64df9d0 to bf14302 Compare October 24, 2023 08:00
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 5 files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)


utils/stack-usage-stat.sh line 17 at r2 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

If you want delete line number from log you can use:
sed -i 's/:[0-9]*:[0-9]//g' src/stat/stack-usage-[non]debug.txt
Before: 16448 src/nondebug/libpmemobj/obj.su:obj.c:549:1:obj_vg_check_no_undef static
After: 16464 src/nondebug/libpmemobj/obj.su:obj.c:obj_vg_check_no_undef static

Done.

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 2 of 2 files at r1, 3 of 3 files at r3, 2 of 5 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @grom72 and @osalyk)


src/libpmem/Makefile line 70 at r3 (raw file):

ifeq ($(OS_DIMM),ndctl)
CFLAGS += $(LIBNDCTL_CFLAGS)
endif

This issue is solved here: #5890. Please remove this part.


utils/stack-usage-stat.sh line 9 at r3 (raw file):

#

if [ ! -d "src/stat" ] ; then

Suggestion:

if [ ! -d "src/stat" ]; then

utils/stack-usage-stat.sh line 10 at r3 (raw file):

if [ ! -d "src/stat" ] ; then
	mkdir src/stat

Please rename the output directory: stat -> stats.


utils/stack-usage-stat.sh line 14 at r3 (raw file):

fgrep -r -e static -e dynamic src | grep '.su:' | \
    grep -e nondebug | \
  1. You are searching all directories in src including stat(s) and tests which is both unnecessary and problematic.
  2. You omitted bounded stack lines.

I think it will be a lot clearer to just skip empty log lines (-v ^$) from *.su files which is dead simple considering we exactly know where they are.

Ref: https://gcc.gnu.org/onlinedocs/gnat_ugn/Static-Stack-Usage-Analysis.html

Suggestion:

grep -v ^$ src/nondebug/*/*.su |\

utils/stack-usage-stat.sh line 15 at r3 (raw file):

fgrep -r -e static -e dynamic src | grep '.su:' | \
    grep -e nondebug | \
    gawk '{print $2 " "  $1 " " $3 " "  $4 " "  $5}' | \

You can do both reordering fields and removing line numbers at the same time.

Suggestion:

gawk -F "[:\t]" '{print $6 " " $1 ":" $2 ":" $5 " " $7}'

utils/stack-usage-stat.sh line 25 at r3 (raw file):

    sort -n -r > src/stat/stack-usage-debug.txt

sed -i 's/:[0-9]*:[0-9]//g' src/stat/stack-usage-debug.txt

And since I do not like redundancy I sincerely recommend using a loop here so all of the above will look like follows.

for build in debug nondebug; do
    grep -v ^$ src/$build/*/*.su |\
        gawk -F "[:\t]" '{print $6 " " $1 ":" $2 ":" $5 " " $7}' |\
        sort -n -r > src/stats/stack-usage-$build.txt
done

@grom72 grom72 force-pushed the fstack-usage-enable branch from aa42768 to 87108b8 Compare October 24, 2023 11:29
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 5 files at r4, 4 of 6 files at r6.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmem/Makefile line 70 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This issue is solved here: #5890. Please remove this part.

Done.


utils/stack-usage-stat.sh line 9 at r3 (raw file):

#

if [ ! -d "src/stat" ] ; then

Done.


utils/stack-usage-stat.sh line 10 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please rename the output directory: stat -> stats.

Done.


utils/stack-usage-stat.sh line 14 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…
  1. You are searching all directories in src including stat(s) and tests which is both unnecessary and problematic.
  2. You omitted bounded stack lines.

I think it will be a lot clearer to just skip empty log lines (-v ^$) from *.su files which is dead simple considering we exactly know where they are.

Ref: https://gcc.gnu.org/onlinedocs/gnat_ugn/Static-Stack-Usage-Analysis.html

Done.


utils/stack-usage-stat.sh line 15 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You can do both reordering fields and removing line numbers at the same time.

Done.


utils/stack-usage-stat.sh line 25 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

And since I do not like redundancy I sincerely recommend using a loop here so all of the above will look like follows.

for build in debug nondebug; do
    grep -v ^$ src/$build/*/*.su |\
        gawk -F "[:\t]" '{print $6 " " $1 ":" $2 ":" $5 " " $7}' |\
        sort -n -r > src/stats/stack-usage-$build.txt
done

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 3 of 5 files at r4.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)

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 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)

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 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72 and @osalyk)


src/libpmem/Makefile line 70 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

No, it's not.


utils/stack-usage-stats.sh line 1 at r6 (raw file):

# SPDX-License-Identifier: BSD-3-Clause

Code snippet:

#!/usr/bin/env bash

utils/stack-usage-stats.sh line 15 at r6 (raw file):

	grep -v ^$ src/$build/*/*.su | \
	gawk -F "[:\t]" '{print $6 " " $5 " : " $1 ":" $2 " " $7}' | \
	sort -n -r > src/stats/stack-usage-$build.txt

Indentation.

Suggestion:

	grep -v ^$ src/$build/*/*.su | \
		gawk -F "[:\t]" '{print $6 " " $5 " : " $1 ":" $2 " " $7}' | \
		sort -n -r > src/stats/stack-usage-$build.txt

@grom72 grom72 force-pushed the fstack-usage-enable branch from 87108b8 to cccf20d Compare October 24, 2023 17:37
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 2 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @janekmi and @osalyk)


utils/stack-usage-stats.sh line 1 at r6 (raw file):

# SPDX-License-Identifier: BSD-3-Clause

Done.


utils/stack-usage-stats.sh line 15 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Indentation.

Done.


src/libpmem/Makefile line 70 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No, it's not.

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 all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @janekmi and @osalyk)

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 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)


src/libpmem/Makefile line 70 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

I believe you have confused commits. The first one is still changing this file whereas the second one reverts its predecessor's change. Either squash them or make it straight.

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
@grom72 grom72 force-pushed the fstack-usage-enable branch from cccf20d to ddb2081 Compare October 25, 2023 04:42
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 4 of 4 files at r8, all commit messages.
Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @osalyk)

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 r1.
Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmem/Makefile line 70 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe you have confused commits. The first one is still changing this file whereas the second one reverts its predecessor's change. Either squash them or make it straight.

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 3 of 3 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

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 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

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 2 of 3 files at r3, 1 of 5 files at r4, 3 of 6 files at r6, 1 of 4 files at r8, 2 of 3 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@grom72 grom72 merged commit b336e0e into pmem:master Oct 25, 2023
5 checks passed
@grom72 grom72 deleted the fstack-usage-enable branch October 25, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DAOS DAOS-related libpmemobj src/libpmemobj no changelog Add to skip the changelog check on your pull request 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