perf tools: Remove redundant variable assignment

[PATCH] perf tools: Remove redundant variable assignment

Posted by Luo Yifan 1 week, 5 days ago

This patch makes a minor change that removes the redundant assignment
to the variable ret, simplifying the code.

Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
---
 tools/perf/jvmti/jvmti_agent.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 526dcaf9f..751219143 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -408,9 +408,7 @@ jvmti_write_code(void *agent, char const *sym,
 
 	funlockfile(fp);
 
-	ret = 0;
-
-	return ret;
+	return 0;
 }
 
 int
-- 
2.27.0

Re: [PATCH] perf tools: Remove redundant variable assignment

Posted by Arnaldo Carvalho de Melo 1 week, 5 days ago

On Mon, Nov 11, 2024 at 04:27:13PM +0800, Luo Yifan wrote:
> This patch makes a minor change that removes the redundant assignment
> to the variable ret, simplifying the code.

Thanks, applied to perf-tools-next,

- Arnaldo
 
> Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
> ---
>  tools/perf/jvmti/jvmti_agent.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
> index 526dcaf9f..751219143 100644
> --- a/tools/perf/jvmti/jvmti_agent.c
> +++ b/tools/perf/jvmti/jvmti_agent.c
> @@ -408,9 +408,7 @@ jvmti_write_code(void *agent, char const *sym,
>  
>  	funlockfile(fp);
>  
> -	ret = 0;
> -
> -	return ret;
> +	return 0;
>  }
>  
>  int
> -- 
> 2.27.0
> 
>

Re: [PATCH] perf tools: Remove redundant variable assignment

Posted by Arnaldo Carvalho de Melo 1 week, 5 days ago

On Mon, Nov 11, 2024 at 02:42:46PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Nov 11, 2024 at 04:27:13PM +0800, Luo Yifan wrote:
> > This patch makes a minor change that removes the redundant assignment
> > to the variable ret, simplifying the code.
> 
> Thanks, applied to perf-tools-next,

Are you build testing these patches?

 GEN     perf-archive
  CC      /tmp/build/perf-tools-next/libsubcmd/sigchain.o
  GEN     perf-iostat
  INSTALL libbpf_headers
  LD      /tmp/build/perf-tools-next/libsubcmd/libsubcmd-in.o
  AR      /tmp/build/perf-tools-next/libsubcmd/libsubcmd.a
jvmti/jvmti_agent.c: In function ‘jvmti_write_code’:
jvmti/jvmti_agent.c:366:13: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
  366 |         int ret = -1;
      |             ^~~
cc1: all warnings being treated as errors
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/jvmti/jvmti_agent.o] Error 1
make[2]: *** [Makefile.perf:936: /tmp/build/perf-tools-next/jvmti/jvmti-in.o] Error 2
make[2]: *** Waiting for unfinished jobs....
  CC      /tmp/build/perf-tools-next/util/header.o
  LD      /tmp/build/perf-tools-next/util/perf-util-in.o
  LD      /tmp/build/perf-tools-next/perf-util-in.o
make[1]: *** [Makefile.perf:292: sub-make] Error 2
make: *** [Makefile:119: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
⬢ [acme@toolbox perf-tools-next]$

The original patch by Stephane has it right, that initial ret = -1 is
used when there are other problems and the code goes to a label at the
end, returning that -1.

But the code was changed later and problems were introduced, so you
removed something simple at the end and somehow missed that it breaks
the build (at least for me) and when I go look at the code, I see the
other problems, so please take the time to try and investigate this and
fix the 'ret' variable usage.

I'm removing this patch from my local tree.

Thanks,
 
> - Arnaldo
>  
> > Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
> > ---
> >  tools/perf/jvmti/jvmti_agent.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
> > index 526dcaf9f..751219143 100644
> > --- a/tools/perf/jvmti/jvmti_agent.c
> > +++ b/tools/perf/jvmti/jvmti_agent.c
> > @@ -408,9 +408,7 @@ jvmti_write_code(void *agent, char const *sym,
> >  
> >  	funlockfile(fp);
> >  
> > -	ret = 0;
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  int
> > -- 
> > 2.27.0
> > 
> >

Re: [PATCH] perf tools: Remove redundant variable assignment

Posted by Arnaldo Carvalho de Melo 1 week, 5 days ago

On Mon, Nov 11, 2024 at 02:51:28PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Nov 11, 2024 at 02:42:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Nov 11, 2024 at 04:27:13PM +0800, Luo Yifan wrote:
> > > This patch makes a minor change that removes the redundant assignment
> > > to the variable ret, simplifying the code.
> > 
> > Thanks, applied to perf-tools-next,
> 
> Are you build testing these patches?
> 
>  GEN     perf-archive
>   CC      /tmp/build/perf-tools-next/libsubcmd/sigchain.o
>   GEN     perf-iostat
>   INSTALL libbpf_headers
>   LD      /tmp/build/perf-tools-next/libsubcmd/libsubcmd-in.o
>   AR      /tmp/build/perf-tools-next/libsubcmd/libsubcmd.a
> jvmti/jvmti_agent.c: In function ‘jvmti_write_code’:
> jvmti/jvmti_agent.c:366:13: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
>   366 |         int ret = -1;
>       |             ^~~
> cc1: all warnings being treated as errors
> make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/jvmti/jvmti_agent.o] Error 1
> make[2]: *** [Makefile.perf:936: /tmp/build/perf-tools-next/jvmti/jvmti-in.o] Error 2
> make[2]: *** Waiting for unfinished jobs....
>   CC      /tmp/build/perf-tools-next/util/header.o
>   LD      /tmp/build/perf-tools-next/util/perf-util-in.o
>   LD      /tmp/build/perf-tools-next/perf-util-in.o
> make[1]: *** [Makefile.perf:292: sub-make] Error 2
> make: *** [Makefile:119: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> ⬢ [acme@toolbox perf-tools-next]$
> 
> The original patch by Stephane has it right, that initial ret = -1 is
> used when there are other problems and the code goes to a label at the
> end, returning that -1.
> 
> But the code was changed later and problems were introduced, so you
> removed something simple at the end and somehow missed that it breaks
> the build (at least for me) and when I go look at the code, I see the
> other problems, so please take the time to try and investigate this and
> fix the 'ret' variable usage.
> 
> I'm removing this patch from my local tree.

Ok, some of the other functions use the label at the end + return ret
and looks nice, but the jvmti_write_code() one has this problem since
day one, just look at the other routines and fix this one, please.

- Arnaldo
 
> Thanks,
>  
> > - Arnaldo
> >  
> > > Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
> > > ---
> > >  tools/perf/jvmti/jvmti_agent.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
> > > index 526dcaf9f..751219143 100644
> > > --- a/tools/perf/jvmti/jvmti_agent.c
> > > +++ b/tools/perf/jvmti/jvmti_agent.c
> > > @@ -408,9 +408,7 @@ jvmti_write_code(void *agent, char const *sym,
> > >  
> > >  	funlockfile(fp);
> > >  
> > > -	ret = 0;
> > > -
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  int
> > > -- 
> > > 2.27.0
> > > 
> > >

Re: [PATCH] perf tools: Remove redundant variable assignment

Posted by Luo Yifan 1 week, 4 days ago

Ok, I'll check what happened and fix it, then resubmit the patch.

[PATCH] perf jvmti: Remove unnecessary ret variable in jvmti_write_code

Posted by Luo Yifan 1 week, 4 days ago

Following the approach in the jvmti_write_debug_info function, just
remove the ret variable from jvmti_write_code function. It's safe since
we don’t really care about the return value of fwrite_unlocked. This
change makes the code cleaner and more compiler-friendly.

Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
---
 tools/perf/jvmti/jvmti_agent.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 526dcaf9f..9b49a880b 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -363,7 +363,6 @@ jvmti_write_code(void *agent, char const *sym,
 	struct jr_code_load rec;
 	size_t sym_len;
 	FILE *fp = agent;
-	int ret = -1;
 
 	/* don't care about 0 length function, no samples */
 	if (size == 0)
@@ -400,7 +399,7 @@ jvmti_write_code(void *agent, char const *sym,
 	 */
 	rec.code_index = code_generation++;
 
-	ret = fwrite_unlocked(&rec, sizeof(rec), 1, fp);
+	fwrite_unlocked(&rec, sizeof(rec), 1, fp);
 	fwrite_unlocked(sym, sym_len, 1, fp);
 
 	if (code)
@@ -408,9 +407,7 @@ jvmti_write_code(void *agent, char const *sym,
 
 	funlockfile(fp);
 
-	ret = 0;
-
-	return ret;
+	return 0;
 }
 
 int
-- 
2.27.0

Re: [PATCH] perf jvmti: Remove unnecessary ret variable in jvmti_write_code

Posted by Luo Yifan 1 week, 4 days ago

I've rechecked the code and found that removing the ret variable is
not a good solution. Please ignore this patch, I'll submit a final
version to fix it properly.

[PATCH] perf jvmti: Properly handle return value checks in jvmti_write_code

Posted by Luo Yifan 1 week, 4 days ago

Following the approach in the jvmti_write_debug_info function, add
some return value checks in jvmti_write_code.

Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
---
 tools/perf/jvmti/jvmti_agent.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 526dcaf9f..b52466a0c 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -361,9 +361,8 @@ jvmti_write_code(void *agent, char const *sym,
 {
 	static int code_generation = 1;
 	struct jr_code_load rec;
-	size_t sym_len;
+	size_t sret, sym_len;
 	FILE *fp = agent;
-	int ret = -1;
 
 	/* don't care about 0 length function, no samples */
 	if (size == 0)
@@ -400,17 +399,25 @@ jvmti_write_code(void *agent, char const *sym,
 	 */
 	rec.code_index = code_generation++;
 
-	ret = fwrite_unlocked(&rec, sizeof(rec), 1, fp);
-	fwrite_unlocked(sym, sym_len, 1, fp);
-
-	if (code)
-		fwrite_unlocked(code, size, 1, fp);
+	sret = fwrite_unlocked(&rec, sizeof(rec), 1, fp);
+	if (sret != 1)
+		goto error;
 
-	funlockfile(fp);
+	sret = fwrite_unlocked(sym, sym_len, 1, fp);
+	if (sret != 1)
+		goto error;
 
-	ret = 0;
+	if (code) {
+		sret = fwrite_unlocked(code, size, 1, fp);
+		if (sret != 1)
+			goto error;
+	}
 
-	return ret;
+	funlockfile(fp);
+	return 0;
+error:
+	funlockfile(fp);
+	return -1;
 }
 
 int
-- 
2.33.0