[PATCH-for-9.0 0/4] overall: Avoid using inlined functions with external linkage again

[PATCH-for-9.0 0/4] overall: Avoid using inlined functions with external linkage again

[Philippe Mathieu-Daudé]

Mostly as a C style cleanup, use -Wstatic-in-inline to avoid
using inlined function with external linkage.

Philippe Mathieu-Daudé (4):
  hw/arm/smmu: Avoid using inlined functions with external linkage again
  accel/hvf: Un-inline hvf_arch_supports_guest_debug()
  qtest/libqos: Un-inline size_to_prdtl()
  meson: Enable -Wstatic-in-inline

 meson.build               | 1 +
 hw/arm/smmu-common.c      | 2 +-
 target/arm/hvf/hvf.c      | 2 +-
 target/i386/hvf/hvf.c     | 2 +-
 tests/qtest/libqos/ahci.c | 2 +-
 5 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 1/4] hw/arm/smmu: Avoid using inlined functions with external linkage again

[Philippe Mathieu-Daudé]

Similarly to commit 9de9fa5cf2 ("hw/arm/smmu-common: Avoid using
inlined functions with external linkage"):

  None of our code base require / use inlined functions with external
  linkage. Some places use internal inlining in the hot path. These
  two functions are certainly not in any hot path and don't justify
  any inlining, so these are likely oversights rather than intentional.

Fix:

  C compiler for the host machine: clang (clang 15.0.0 "Apple clang version 15.0.0 (clang-1500.3.9.4)")
  ...
  hw/arm/smmu-common.c:203:43: error: static function 'smmu_hash_remove_by_vmid' is
  used in an inline function with external linkage [-Werror,-Wstatic-in-inline]
      g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
                                            ^
  include/hw/arm/smmu-common.h:197:1: note: use 'static' to give inline function 'smmu_iotlb_inv_vmid' internal linkage
  void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
  ^
  static
  hw/arm/smmu-common.c:139:17: note: 'smmu_hash_remove_by_vmid' declared here
  static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
                ^

Fixes: ccc3ee3871 ("hw/arm/smmuv3: Add CMDs related to stage-2")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/smmu-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 4caedb4998..c4b540656c 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -197,7 +197,7 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
     g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
 }
 
-inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
+void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
 {
     trace_smmu_iotlb_inv_vmid(vmid);
     g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
-- 
2.41.0

[PATCH-for-9.0 2/4] accel/hvf: Un-inline hvf_arch_supports_guest_debug()

[Philippe Mathieu-Daudé]

See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
functions with external linkage") for rationale.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/hvf/hvf.c  | 2 +-
 target/i386/hvf/hvf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index e5f0f60093..65a5601804 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -2246,7 +2246,7 @@ void hvf_arch_update_guest_debug(CPUState *cpu)
     hvf_arch_set_traps();
 }
 
-inline bool hvf_arch_supports_guest_debug(void)
+bool hvf_arch_supports_guest_debug(void)
 {
     return true;
 }
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 11ffdd4c69..1ed8ed5154 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -708,7 +708,7 @@ void hvf_arch_update_guest_debug(CPUState *cpu)
 {
 }
 
-inline bool hvf_arch_supports_guest_debug(void)
+bool hvf_arch_supports_guest_debug(void)
 {
     return false;
 }
-- 
2.41.0

[PATCH-for-9.0 3/4] qtest/libqos: Un-inline size_to_prdtl()

[Philippe Mathieu-Daudé]

See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
functions with external linkage") for rationale.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/libqos/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index a2c94c6e06..135b23ffd9 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -662,7 +662,7 @@ unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port)
     g_assert_not_reached();
 }
 
-inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
+unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
 {
     /* Each PRD can describe up to 4MiB */
     g_assert_cmphex(bytes_per_prd, <=, 4096 * 1024);
-- 
2.41.0

[PATCH-for-9.0 4/4] meson: Enable -Wstatic-in-inline

[Philippe Mathieu-Daudé]

Compilers are clever enough to inline code when necessary.

The only case we accept an inline function is static in
header (we use C, not C++).

Add the -Wstatic-in-inline CPPFLAG to prevent public and
inline function to be added in the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index b375248a76..f57397aa53 100644
--- a/meson.build
+++ b/meson.build
@@ -591,6 +591,7 @@ warn_flags = [
   '-Wold-style-definition',
   '-Wredundant-decls',
   '-Wshadow=local',
+  '-Wstatic-in-inline',
   '-Wstrict-prototypes',
   '-Wtype-limits',
   '-Wundef',
-- 
2.41.0

Re: [PATCH-for-9.0 3/4] qtest/libqos: Un-inline size_to_prdtl()

[Peter Maydell]

On Wed, 13 Mar 2024 at 18:50, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
> functions with external linkage") for rationale.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/qtest/libqos/ahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index a2c94c6e06..135b23ffd9 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -662,7 +662,7 @@ unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port)
>      g_assert_not_reached();
>  }
>
> -inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
> +unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
>  {
>      /* Each PRD can describe up to 4MiB */
>      g_assert_cmphex(bytes_per_prd, <=, 4096 * 1024);

It looks like this function is only used in this file, so
we could make it static ?

-- PMM

Re: [PATCH-for-9.0 2/4] accel/hvf: Un-inline hvf_arch_supports_guest_debug()

[Peter Maydell]

On Wed, 13 Mar 2024 at 18:50, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
> functions with external linkage") for rationale.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH-for-9.0 1/4] hw/arm/smmu: Avoid using inlined functions with external linkage again

[Richard Henderson]

On 3/13/24 08:49, Philippe Mathieu-Daudé wrote:
> Similarly to commit 9de9fa5cf2 ("hw/arm/smmu-common: Avoid using
> inlined functions with external linkage"):
> 
>    None of our code base require / use inlined functions with external
>    linkage. Some places use internal inlining in the hot path. These
>    two functions are certainly not in any hot path and don't justify
>    any inlining, so these are likely oversights rather than intentional.
> 
> Fix:
> 
>    C compiler for the host machine: clang (clang 15.0.0 "Apple clang version 15.0.0 (clang-1500.3.9.4)")
>    ...
>    hw/arm/smmu-common.c:203:43: error: static function 'smmu_hash_remove_by_vmid' is
>    used in an inline function with external linkage [-Werror,-Wstatic-in-inline]
>        g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
>                                              ^
>    include/hw/arm/smmu-common.h:197:1: note: use 'static' to give inline function 'smmu_iotlb_inv_vmid' internal linkage
>    void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
>    ^
>    static
>    hw/arm/smmu-common.c:139:17: note: 'smmu_hash_remove_by_vmid' declared here
>    static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
>                  ^
> 
> Fixes: ccc3ee3871 ("hw/arm/smmuv3: Add CMDs related to stage-2")
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/arm/smmu-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.0 2/4] accel/hvf: Un-inline hvf_arch_supports_guest_debug()

[Richard Henderson]

On 3/13/24 08:49, Philippe Mathieu-Daudé wrote:
> See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
> functions with external linkage") for rationale.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c  | 2 +-
>   target/i386/hvf/hvf.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.0 4/4] meson: Enable -Wstatic-in-inline

[Richard Henderson]

On 3/13/24 08:49, Philippe Mathieu-Daudé wrote:
> Compilers are clever enough to inline code when necessary.
> 
> The only case we accept an inline function is static in
> header (we use C, not C++).
> 
> Add the -Wstatic-in-inline CPPFLAG to prevent public and
> inline function to be added in the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.0 3/4] qtest/libqos: Un-inline size_to_prdtl()

[Philippe Mathieu-Daudé]

On 13/3/24 20:39, Peter Maydell wrote:
> On Wed, 13 Mar 2024 at 18:50, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> See previous commit and commit 9de9fa5cf2 ("Avoid using inlined
>> functions with external linkage") for rationale.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/libqos/ahci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
>> index a2c94c6e06..135b23ffd9 100644
>> --- a/tests/qtest/libqos/ahci.c
>> +++ b/tests/qtest/libqos/ahci.c
>> @@ -662,7 +662,7 @@ unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port)
>>       g_assert_not_reached();
>>   }
>>
>> -inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
>> +unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
>>   {
>>       /* Each PRD can describe up to 4MiB */
>>       g_assert_cmphex(bytes_per_prd, <=, 4096 * 1024);
> 
> It looks like this function is only used in this file, so
> we could make it static ?

Eh I did check for that, but sure how I missed it...