| Message ID | 20231030163834.4638-1-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: qtest already exports qtest_rtas_call() | expand |
On 10/30/23 17:38, Juan Quintela wrote: > Having two functions with the same name is a bad idea. As spapr only > uses the function locally, made it static. > > When you compile with clang, you get this compilation error: > > /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': > /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: multiple definition of `qtest_rtas_call'; libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: first defined here > clang-16: error: linker command failed with exit code 1 (use -v to see invocation) > ninja: build stopped: subcommand failed. > make: *** [Makefile:162: run-ninja] Error 1 > > Signed-off-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > include/hw/ppc/spapr_rtas.h | 10 ---------- > hw/ppc/spapr_rtas.c | 4 ++-- > 2 files changed, 2 insertions(+), 12 deletions(-) > delete mode 100644 include/hw/ppc/spapr_rtas.h > > diff --git a/include/hw/ppc/spapr_rtas.h b/include/hw/ppc/spapr_rtas.h > deleted file mode 100644 > index 383611f10f..0000000000 > --- a/include/hw/ppc/spapr_rtas.h > +++ /dev/null > @@ -1,10 +0,0 @@ > -#ifndef HW_SPAPR_RTAS_H > -#define HW_SPAPR_RTAS_H > -/* > - * This work is licensed under the terms of the GNU GPL, version 2 or later. > - * See the COPYING file in the top-level directory. > - */ > - > -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > - uint32_t nret, uint64_t rets); > -#endif /* HW_SPAPR_RTAS_H */ > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 26c384b261..cec01b2c92 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -531,8 +531,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, SpaprMachineState *spapr, > return H_PARAMETER; > } > > -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > - uint32_t nret, uint64_t rets) > +static uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > + uint32_t nret, uint64_t rets) > { > int token; >
On Mon, Oct 30, 2023 at 05:41:36PM +0100, Cédric le Goater wrote: > On 10/30/23 17:38, Juan Quintela wrote: > > Having two functions with the same name is a bad idea. As spapr only > > uses the function locally, made it static. > > > > When you compile with clang, you get this compilation error: > > > > /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': > > /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: multiple definition of `qtest_rtas_call'; libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: first defined here > > clang-16: error: linker command failed with exit code 1 (use -v to see invocation) > > ninja: build stopped: subcommand failed. > > make: *** [Makefile:162: run-ninja] Error 1 > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> I think changing the name of one of the functions would be even better. Making it static means it won't confuse the compiler, but it can still confuse people.
David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Oct 30, 2023 at 05:41:36PM +0100, Cédric le Goater wrote: >> On 10/30/23 17:38, Juan Quintela wrote: >> > Having two functions with the same name is a bad idea. As spapr only >> > uses the function locally, made it static. >> > >> > When you compile with clang, you get this compilation error: >> > >> > /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': >> > /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: >> > multiple definition of `qtest_rtas_call'; >> > libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: >> > first defined here >> > clang-16: error: linker command failed with exit code 1 (use -v to see invocation) >> > ninja: build stopped: subcommand failed. >> > make: *** [Makefile:162: run-ninja] Error 1 >> > >> > Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> > > I think changing the name of one of the functions would be even > better. Making it static means it won't confuse the compiler, but it > can still confuse people. I think that made it static when it is not used anywhere else is a good idea. After that, I don't understand it enough to make a rename that makes sense. This patch is the typical fix for "make all" with clang fails here. I let ppc maintainers to do anything more sensible. Later, Juan.
On 10/31/23 11:15, Juan Quintela wrote: > David Gibson <david@gibson.dropbear.id.au> wrote: >> On Mon, Oct 30, 2023 at 05:41:36PM +0100, Cédric le Goater wrote: >>> On 10/30/23 17:38, Juan Quintela wrote: >>>> Having two functions with the same name is a bad idea. As spapr only >>>> uses the function locally, made it static. >>>> >>>> When you compile with clang, you get this compilation error: >>>> >>>> /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': >>>> /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: >>>> multiple definition of `qtest_rtas_call'; >>>> libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: >>>> first defined here >>>> clang-16: error: linker command failed with exit code 1 (use -v to see invocation) >>>> ninja: build stopped: subcommand failed. >>>> make: *** [Makefile:162: run-ninja] Error 1 >>>> >>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> >>> >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> I think changing the name of one of the functions would be even >> better. Making it static means it won't confuse the compiler, but it >> can still confuse people. Since it is a spapr file, something like : static uint64_t spapr_qtest_rtas_call(...) ? Thanks, C. > I think that made it static when it is not used anywhere else is a good > idea. > > After that, I don't understand it enough to make a rename that makes > sense. > > This patch is the typical fix for "make all" with clang fails here. > I let ppc maintainers to do anything more sensible. > > Later, Juan. >
Queue in ppc-next after amending this in: $ git diff diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index cec01b2c92..f329693c55 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -38,7 +38,6 @@ #include "hw/ppc/spapr.h" #include "hw/ppc/spapr_vio.h" -#include "hw/ppc/spapr_rtas.h" #include "hw/ppc/spapr_cpu_core.h" #include "hw/ppc/ppc.h" Gitlab complained that we were including a non-existing header. Thanks, Daniel On 10/30/23 13:38, Juan Quintela wrote: > Having two functions with the same name is a bad idea. As spapr only > uses the function locally, made it static. > > When you compile with clang, you get this compilation error: > > /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': > /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: multiple definition of `qtest_rtas_call'; libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: first defined here > clang-16: error: linker command failed with exit code 1 (use -v to see invocation) > ninja: build stopped: subcommand failed. > make: *** [Makefile:162: run-ninja] Error 1 > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > include/hw/ppc/spapr_rtas.h | 10 ---------- > hw/ppc/spapr_rtas.c | 4 ++-- > 2 files changed, 2 insertions(+), 12 deletions(-) > delete mode 100644 include/hw/ppc/spapr_rtas.h > > diff --git a/include/hw/ppc/spapr_rtas.h b/include/hw/ppc/spapr_rtas.h > deleted file mode 100644 > index 383611f10f..0000000000 > --- a/include/hw/ppc/spapr_rtas.h > +++ /dev/null > @@ -1,10 +0,0 @@ > -#ifndef HW_SPAPR_RTAS_H > -#define HW_SPAPR_RTAS_H > -/* > - * This work is licensed under the terms of the GNU GPL, version 2 or later. > - * See the COPYING file in the top-level directory. > - */ > - > -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > - uint32_t nret, uint64_t rets); > -#endif /* HW_SPAPR_RTAS_H */ > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 26c384b261..cec01b2c92 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -531,8 +531,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, SpaprMachineState *spapr, > return H_PARAMETER; > } > > -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > - uint32_t nret, uint64_t rets) > +static uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, > + uint32_t nret, uint64_t rets) > { > int token; >
diff --git a/include/hw/ppc/spapr_rtas.h b/include/hw/ppc/spapr_rtas.h deleted file mode 100644 index 383611f10f..0000000000 --- a/include/hw/ppc/spapr_rtas.h +++ /dev/null @@ -1,10 +0,0 @@ -#ifndef HW_SPAPR_RTAS_H -#define HW_SPAPR_RTAS_H -/* - * This work is licensed under the terms of the GNU GPL, version 2 or later. - * See the COPYING file in the top-level directory. - */ - -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, - uint32_t nret, uint64_t rets); -#endif /* HW_SPAPR_RTAS_H */ diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 26c384b261..cec01b2c92 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -531,8 +531,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, SpaprMachineState *spapr, return H_PARAMETER; } -uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, - uint32_t nret, uint64_t rets) +static uint64_t qtest_rtas_call(char *cmd, uint32_t nargs, uint64_t args, + uint32_t nret, uint64_t rets) { int token;
Having two functions with the same name is a bad idea. As spapr only uses the function locally, made it static. When you compile with clang, you get this compilation error: /usr/bin/ld: tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o: in function `qtest_rtas_call': /scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1195: multiple definition of `qtest_rtas_call'; libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_rtas.c.o:/scratch/qemu/clang/full/all/../../../../../mnt/code/qemu/full/hw/ppc/spapr_rtas.c:536: first defined here clang-16: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed. make: *** [Makefile:162: run-ninja] Error 1 Signed-off-by: Juan Quintela <quintela@redhat.com> --- include/hw/ppc/spapr_rtas.h | 10 ---------- hw/ppc/spapr_rtas.c | 4 ++-- 2 files changed, 2 insertions(+), 12 deletions(-) delete mode 100644 include/hw/ppc/spapr_rtas.h