Avoid `perf bench internals inject-build-id` referencing the
cmd_inject sub-command that requires perf-bench to backward reference
internals of builtins. Replace the reference to cmd_inject with a call
to main. To avoid python.c needing to link with something providing
main, drop the libperf-bench library from the python shared object.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Makefile.perf          |  7 +++++--
 tools/perf/bench/inject-buildid.c | 13 +++++++------
 tools/perf/util/python.c          |  6 ------
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 105f734b6820..e54c6953cf02 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -487,6 +487,9 @@ endif
 EXTLIBS := $(call filter-out,$(EXCLUDE_EXTLIBS),$(EXTLIBS))
 LIBS = -Wl,--whole-archive $(PERFLIBS) $(EXTRA_PERFLIBS) -Wl,--no-whole-archive -Wl,--start-group $(EXTLIBS) -Wl,--end-group
 
+PERFLIBS_PY := $(call filter-out,$(LIBPERF_BENCH),$(PERFLIBS))
+LIBS_PY = -Wl,--whole-archive $(PERFLIBS_PY) $(EXTRA_PERFLIBS) -Wl,--no-whole-archive -Wl,--start-group $(EXTLIBS) -Wl,--end-group
+
 export INSTALL SHELL_PATH
 
 ### Build rules
@@ -735,9 +738,9 @@ all: shell_compatibility_test $(ALL_PROGRAMS) $(LANG_BINDINGS) $(OTHER_PROGRAMS)
 # Create python binding output directory if not already present
 $(shell [ -d '$(OUTPUT)python' ] || mkdir -p '$(OUTPUT)python')
 
-$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): util/python.c util/setup.py $(PERFLIBS)
+$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): util/python.c util/setup.py $(PERFLIBS_PY)
 	$(QUIET_GEN)LDSHARED="$(CC) -pthread -shared" \
-        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS) $(LIBS)' \
+        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS) $(LIBS_PY)' \
 	  $(PYTHON_WORD) util/setup.py \
 	  --quiet build_ext; \
 	cp $(PYTHON_EXTBUILD_LIB)perf*.so $(OUTPUT)python/
diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
index a759eb2328be..f55c07e4be94 100644
--- a/tools/perf/bench/inject-buildid.c
+++ b/tools/perf/bench/inject-buildid.c
@@ -52,7 +52,7 @@ struct bench_dso {
 static int nr_dsos;
 static struct bench_dso *dsos;
 
-extern int cmd_inject(int argc, const char *argv[]);
+extern int main(int argc, const char **argv);
 
 static const struct option options[] = {
 	OPT_UINTEGER('i', "iterations", &iterations,
@@ -294,7 +294,7 @@ static int setup_injection(struct bench_data *data, bool build_id_all)
 
 	if (data->pid == 0) {
 		const char **inject_argv;
-		int inject_argc = 2;
+		int inject_argc = 3;
 
 		close(data->input_pipe[1]);
 		close(data->output_pipe[0]);
@@ -318,15 +318,16 @@ static int setup_injection(struct bench_data *data, bool build_id_all)
 		if (inject_argv == NULL)
 			exit(1);
 
-		inject_argv[0] = strdup("inject");
-		inject_argv[1] = strdup("-b");
+		inject_argv[0] = strdup("perf");
+		inject_argv[1] = strdup("inject");
+		inject_argv[2] = strdup("-b");
 		if (build_id_all)
-			inject_argv[2] = strdup("--buildid-all");
+			inject_argv[3] = strdup("--buildid-all");
 
 		/* signal that we're ready to go */
 		close(ready_pipe[1]);
 
-		cmd_inject(inject_argc, inject_argv);
+		main(inject_argc, inject_argv);
 
 		exit(0);
 	}
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 91fd444615cd..c52da509ae58 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -19,7 +19,6 @@
 #include "util/kwork.h"
 #include "util/sample.h"
 #include <internal/lib.h>
-#include "../builtin.h"
 
 #define _PyUnicode_FromString(arg) \
   PyUnicode_FromString(arg)
@@ -1309,8 +1308,3 @@ struct kwork_work *perf_kwork_add_work(struct perf_kwork *kwork __maybe_unused,
 {
 	return NULL;
 }
-
-int cmd_inject(int argc __maybe_unused, const char *argv[] __maybe_unused)
-{
-	return -1;
-}
-- 
2.47.0.rc1.288.g06298d1525-goog

On Tue, Oct 15, 2024 at 09:24:12PM -0700, Ian Rogers wrote:
> Avoid `perf bench internals inject-build-id` referencing the
> cmd_inject sub-command that requires perf-bench to backward reference
> internals of builtins. Replace the reference to cmd_inject with a call
> to main. To avoid python.c needing to link with something providing
> main, drop the libperf-bench library from the python shared object.

Looks like a clever trick!  But I guess by removing libperf-bench from
the python object, you can remove the reference to cmd_inject, right?

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/Makefile.perf          |  7 +++++--
>  tools/perf/bench/inject-buildid.c | 13 +++++++------
>  tools/perf/util/python.c          |  6 ------
>  3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 105f734b6820..e54c6953cf02 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -487,6 +487,9 @@ endif
>  EXTLIBS := $(call filter-out,$(EXCLUDE_EXTLIBS),$(EXTLIBS))
>  LIBS = -Wl,--whole-archive $(PERFLIBS) $(EXTRA_PERFLIBS) -Wl,--no-whole-archive -Wl,--start-group $(EXTLIBS) -Wl,--end-group
>  
> +PERFLIBS_PY := $(call filter-out,$(LIBPERF_BENCH),$(PERFLIBS))
> +LIBS_PY = -Wl,--whole-archive $(PERFLIBS_PY) $(EXTRA_PERFLIBS) -Wl,--no-whole-archive -Wl,--start-group $(EXTLIBS) -Wl,--end-group
> +
>  export INSTALL SHELL_PATH
>  
>  ### Build rules
> @@ -735,9 +738,9 @@ all: shell_compatibility_test $(ALL_PROGRAMS) $(LANG_BINDINGS) $(OTHER_PROGRAMS)
>  # Create python binding output directory if not already present
>  $(shell [ -d '$(OUTPUT)python' ] || mkdir -p '$(OUTPUT)python')
>  
> -$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): util/python.c util/setup.py $(PERFLIBS)
> +$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): util/python.c util/setup.py $(PERFLIBS_PY)
>  	$(QUIET_GEN)LDSHARED="$(CC) -pthread -shared" \
> -        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS) $(LIBS)' \
> +        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS) $(LIBS_PY)' \
>  	  $(PYTHON_WORD) util/setup.py \
>  	  --quiet build_ext; \
>  	cp $(PYTHON_EXTBUILD_LIB)perf*.so $(OUTPUT)python/
> diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
> index a759eb2328be..f55c07e4be94 100644
> --- a/tools/perf/bench/inject-buildid.c
> +++ b/tools/perf/bench/inject-buildid.c
> @@ -52,7 +52,7 @@ struct bench_dso {
>  static int nr_dsos;
>  static struct bench_dso *dsos;
>  
> -extern int cmd_inject(int argc, const char *argv[]);
> +extern int main(int argc, const char **argv);
>  
>  static const struct option options[] = {
>  	OPT_UINTEGER('i', "iterations", &iterations,
> @@ -294,7 +294,7 @@ static int setup_injection(struct bench_data *data, bool build_id_all)
>  
>  	if (data->pid == 0) {
>  		const char **inject_argv;
> -		int inject_argc = 2;
> +		int inject_argc = 3;
>  
>  		close(data->input_pipe[1]);
>  		close(data->output_pipe[0]);
> @@ -318,15 +318,16 @@ static int setup_injection(struct bench_data *data, bool build_id_all)
>  		if (inject_argv == NULL)
>  			exit(1);
>  
> -		inject_argv[0] = strdup("inject");
> -		inject_argv[1] = strdup("-b");
> +		inject_argv[0] = strdup("perf");
> +		inject_argv[1] = strdup("inject");
> +		inject_argv[2] = strdup("-b");
>  		if (build_id_all)
> -			inject_argv[2] = strdup("--buildid-all");
> +			inject_argv[3] = strdup("--buildid-all");
>  
>  		/* signal that we're ready to go */
>  		close(ready_pipe[1]);
>  
> -		cmd_inject(inject_argc, inject_argv);
> +		main(inject_argc, inject_argv);
>  
>  		exit(0);
>  	}
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 91fd444615cd..c52da509ae58 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -19,7 +19,6 @@
>  #include "util/kwork.h"
>  #include "util/sample.h"
>  #include <internal/lib.h>
> -#include "../builtin.h"
>  
>  #define _PyUnicode_FromString(arg) \
>    PyUnicode_FromString(arg)
> @@ -1309,8 +1308,3 @@ struct kwork_work *perf_kwork_add_work(struct perf_kwork *kwork __maybe_unused,
>  {
>  	return NULL;
>  }
> -
> -int cmd_inject(int argc __maybe_unused, const char *argv[] __maybe_unused)
> -{
> -	return -1;
> -}
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
>

On Mon, Oct 21, 2024 at 11:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Oct 15, 2024 at 09:24:12PM -0700, Ian Rogers wrote:
> > Avoid `perf bench internals inject-build-id` referencing the
> > cmd_inject sub-command that requires perf-bench to backward reference
> > internals of builtins. Replace the reference to cmd_inject with a call
> > to main. To avoid python.c needing to link with something providing
> > main, drop the libperf-bench library from the python shared object.
>
> Looks like a clever trick!  But I guess by removing libperf-bench from
> the python object, you can remove the reference to cmd_inject, right?

So this change is looking to clean up cmd_inject yes, an alternative
thing to do is to remove libperf-bench.a as a python dependency but
that would use an unused stub. I'm looking to remove all the stubs.
Once you start cleaning up the cmd_inject stub then you get where this
patch is.

Wrt cmd_inject have 4 choices:
1) status quo - have libperf-bench.a be depended on by builtin-bench
but have a cycle where libperf-bench.a depends on builtin-inject. This
can yield build issues I've wrestled with in the past and in general
this shouldn't be done.
2) this patch - have a dependency from libperf-bench.a to main in
perf.c, which is less of an internal dependency on the perf.c/builtin
code but is still an annoying cycle. It's not much better than (1) but
at least it shouldn't require a #include "../builtin.h" which is
obviously bad. That #include isn't in the bench code as it re-declares
the function, again bad. The patch here declares main which isn't
great, but at least the declaration of main should not be subject to
change whereas the builtins could be.
3) move perf inject into util or some other 3rd library. Out of scope
for this change, but also not something I think we want.
4) have `perf bench internals inject-build-id` fork/exec the perf
command. My preference but out of scope here.

So I went with 2 to improve things but without doing some larger
rewrite. The cleanups all relate to removing cyclic dependencies to
cmd_inject hence 1 patch.

Thanks,
Ian