-
Notifications
You must be signed in to change notification settings - Fork 511
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
Conversation
Codecov Report
@@ 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 |
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.
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
64df9d0
to
bf14302
Compare
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.
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.
bf14302
to
4862b68
Compare
4862b68
to
aa42768
Compare
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.
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 | \
- You are searching all directories in
src
includingstat(s)
andtests
which is both unnecessary and problematic. - 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
aa42768
to
87108b8
Compare
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.
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…
- You are searching all directories in
src
includingstat(s)
andtests
which is both unnecessary and problematic.- 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.
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.
Reviewed 3 of 5 files at r4.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
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.
Reviewed 2 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi and @osalyk)
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.
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
87108b8
to
cccf20d
Compare
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.
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.
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @janekmi and @osalyk)
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.
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>
cccf20d
to
ddb2081
Compare
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.
Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @osalyk)
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.
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.
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.
Reviewed 3 of 3 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)
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.
Reviewed 1 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)
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.
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: complete! all files reviewed, all discussions resolved (waiting on @grom72)
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