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

uftrace: Fix dead store warnings found by infer #1757

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

paranlee
Copy link
Contributor

@paranlee paranlee commented Jul 12, 2023

uftrace: Fix dead store warnings found by infer

This patch fixes some dead store warnings found by a static analysis
tool, called infer[1].

The warning messages are as follows.

  $ cd /path/to/uftrace
  $ infer run --force-integration make
          ...
  utils/debug.c:386: error: Dead Store
    The value written to `&percent` (type `double`) is never used.
    384. void print_diff_percent(uint64_t base_nsec, uint64_t pair_nsec)
    385. {
    386. 	double percent = 999.99;
          ^

  utils/debug.c:387: error: Dead Store
    The value written to `&sc` is never used.
    385. {
    386.  double percent = 999.99;
    387.  const char *sc = get_color(COLOR_CODE_NORMAL);
          ^
    388.  const char *ec = get_color(COLOR_CODE_RESET);
    389.

  utils/kernel.c:57: error: Dead Store
    The value written to `&ret` is never used.
    55. {
    56.   struct kfilter *pos, *tmp;
    57.   int ret = -1;
         ^
    58.   int fd;
    59.

  utils/perf.c:230: error: Dead Store
    The value written to `&start` is never used.
    228.  buf = &data[start & mask];
    229.  size = end - start;
    230.  start += size;
          ^

[1] https://github.com/facebook/infer

Fixed: #1553
Signed-off-by: Paran Lee p4ranlee@gmail.com

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.

LGTM

Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

Please squash the change at #1758 into this PR.

@paranlee paranlee force-pushed the utils-dead-store-clean-up branch 2 times, most recently from 4e78ca1 to 282079b Compare August 15, 2023 12:17
@honggyukim
Copy link
Collaborator

Please rewrite the commit message by looking at the other commit messages especially as follows.

  1. use Fixed: #1553
  2. write the commit message in complete sentences

@paranlee paranlee force-pushed the utils-dead-store-clean-up branch from 282079b to 4d0eddf Compare August 15, 2023 13:13
@honggyukim
Copy link
Collaborator

The commit message doesn't follow our style again.

The description Remove Not needed initialization on double variable. is not a complete sentence and upper case shouldn't be appeared in the middle of sentence.

Fixed: #1553 is better to be placed after a single blank line. Please read other commit message by running git log and see what is different in your commit message from others.

@honggyukim
Copy link
Collaborator

In addition, the current commit message only explains about the first change.

diff --git a/utils/debug.c b/utils/debug.c
index bbf526fe2..3d779570d 100644
--- a/utils/debug.c
+++ b/utils/debug.c
@@ -383,7 +383,7 @@ void print_time_unit(uint64_t delta_nsec)
 
 void print_diff_percent(uint64_t base_nsec, uint64_t pair_nsec)
 {
-	double percent = 999.99;
+	double percent;
 	const char *sc = get_color(COLOR_CODE_NORMAL);
 	const char *ec = get_color(COLOR_CODE_RESET);

There is no explanation about the second change below.

diff --git a/utils/kernel.c b/utils/kernel.c
index 3a33b67e1..2d2d277b3 100644
--- a/utils/kernel.c
+++ b/utils/kernel.c
@@ -54,7 +54,6 @@ struct kfilter {
 static int set_filter_file(const char *filter_file, struct list_head *filters)
 {
 	struct kfilter *pos, *tmp;
-	int ret = -1;
 	int fd;
 
 	fd = open_tracing_file(filter_file, true);
@@ -75,10 +74,9 @@ static int set_filter_file(const char *filter_file, struct list_head *filters)
 		if (write(fd, " ", 1) != 1)
 			pr_dbg2("writing filter file failed, but ignoring...\n");
 	}
-	ret = 0;
 
 	close(fd);
-	return ret;
+	return 0;
 }
 
 static int set_tracing_filter(struct uftrace_kernel_writer *kernel)

I would rather write the description as follows.

Subject: [PATCH] uftrace: Fix dead store warnings found by infer

This patch fixes some dead store warnings found by a static analysis
tool, called infer[1].

The warning messages are as follows.

  $ cd /path/to/uftrace
  $ infer run --force-integration make
          ...
  #24
  utils/debug.c:386: error: Dead Store
    The value written to `&percent` (type `double`) is never used.
    384. void print_diff_percent(uint64_t base_nsec, uint64_t pair_nsec)
    385. {
    386. 	double percent = 999.99;
          ^

  #28
  utils/kernel.c:57: error: Dead Store
    The value written to `&ret` (type `int`) is never used.
    55. {
    56. 	struct kfilter *pos, *tmp;
    57. 	int ret = -1;
         ^
    58. 	int fd;
    59. 

[1] https://github.com/facebook/infer

Fixed: #1553
Signed-off-by: Paran Lee <p4ranlee@gmail.com>

Please write the commit message with meaningful information.

@paranlee paranlee force-pushed the utils-dead-store-clean-up branch 3 times, most recently from 1c9b677 to d756979 Compare December 10, 2023 21:40
@paranlee paranlee changed the title utils: dead store clean up uftrace: Fix dead store warnings found by infer Dec 10, 2023
@namhyung
Copy link
Owner

Can you please remove something like #24 in the commit message. They make references to unrelated issues.

utils/kernel-parser.c Outdated Show resolved Hide resolved
@paranlee paranlee force-pushed the utils-dead-store-clean-up branch from d756979 to ca0042c Compare December 14, 2023 11:39
@paranlee
Copy link
Contributor Author

@namhyung Thank you for the code review! I amended a new patch. :)

@paranlee paranlee changed the title uftrace: Fix dead store warnings found by infer uftrace: Fix dead store warnings found by infer and warning on compile time implicit declaration Dec 14, 2023
@paranlee paranlee force-pushed the utils-dead-store-clean-up branch from ca0042c to 0c813ce Compare December 14, 2023 11:58
Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

The current commit message title is too long and it's better not to exceed more than 50 or 72 characters.
https://www.gitkraken.com/learn/git/best-practices/git-commit-message#git-commit-message-structure

utils/kernel-parser.c Outdated Show resolved Hide resolved
utils/debug.c Outdated Show resolved Hide resolved
@paranlee paranlee force-pushed the utils-dead-store-clean-up branch 3 times, most recently from b686c13 to ec0ad25 Compare December 14, 2023 12:37
Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

The current commit message title is too long and it's better not to exceed more than 50 or 72 characters.
https://www.gitkraken.com/learn/git/best-practices/git-commit-message#git-commit-message-structure

The commit title is still too long so make it short.

uftrace: Fix dead store warnings found by infer and warning on compile time implicit declaration

utils/kernel-parser.c Outdated Show resolved Hide resolved
This patch fixes some dead store warnings found by a static analysis
tool, called infer[1].

The warning messages are as follows.

  $ cd /path/to/uftrace
  $ infer run --force-integration make
          ...

  utils/debug.c:386: error: Dead Store
    The value written to `&percent` (type `double`) is never used.
    384. void print_diff_percent(uint64_t base_nsec, uint64_t pair_nsec)
    385. {
    386. 	double percent = 999.99;
          ^

  utils/debug.c:387: error: Dead Store
    The value written to `&sc` is never used.
    385. {
    386.  double percent = 999.99;
    387.  const char *sc = get_color(COLOR_CODE_NORMAL);
          ^
    388.  const char *ec = get_color(COLOR_CODE_RESET);
    389.

  utils/kernel.c:57: error: Dead Store
    The value written to `&ret` is never used.
    55. {
    56.   struct kfilter *pos, *tmp;
    57.   int ret = -1;
         ^
    58.   int fd;
    59.

  utils/perf.c:230: error: Dead Store
    The value written to `&start` is never used.
    228.  buf = &data[start & mask];
    229.  size = end - start;
    230.  start += size;
          ^

[1] https://github.com/facebook/infer

Fixed: namhyung#1553
Signed-off-by: Paran Lee <p4ranlee@gmail.com>
@paranlee paranlee force-pushed the utils-dead-store-clean-up branch from ec0ad25 to 930f24d Compare December 16, 2023 23:45
@paranlee paranlee changed the title uftrace: Fix dead store warnings found by infer and warning on compile time implicit declaration uftrace: Fix dead store warnings found by infer Dec 16, 2023
Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

LGTM!

@namhyung namhyung merged commit df5cf62 into namhyung:master Dec 17, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code static checks infered by Infer(x86_64)
3 participants