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

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

[Philippe Mathieu-Daudé]

Oops, I forgot patch 3 needed rework :/ Is it too late to
enforce that flag for 9.0?

Missing review: #3

Since v1:
- Reduce size_to_prdtl() scope

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: Reduce size_to_prdtl() declaration scope
  meson: Enable -Wstatic-in-inline

 meson.build               | 1 +
 tests/qtest/libqos/ahci.h | 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 +-
 6 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH-for-9.0? v2 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20240313184954.42513-2-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

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

[Eric Auger]

Hi,

On 3/26/24 18:10, 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20240313184954.42513-2-philmd@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  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);

[PATCH-for-9.0? v2 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20240313184954.42513-3-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? v2 3/4] qtest/libqos: Reduce size_to_prdtl() declaration scope

[Philippe Mathieu-Daudé]

Since size_to_prdtl() is only used within ahci.c,
declare it statically. This removes the last use
of "inlined function with external linkage". See
previous commit and commit 9de9fa5cf2 for rationale.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/libqos/ahci.h | 1 -
 tests/qtest/libqos/ahci.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index 48017864bf..a0487a1557 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -599,7 +599,6 @@ void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
 
 /* Misc */
 bool is_atapi(AHCIQState *ahci, uint8_t port);
-unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd);
 
 /* Command: Macro level execution */
 void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index a2c94c6e06..6d59c7551a 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)
+static 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

Re: [PATCH-for-9.0? v2 3/4] qtest/libqos: Reduce size_to_prdtl() declaration scope

[Peter Maydell]

On Tue, 26 Mar 2024 at 17:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Since size_to_prdtl() is only used within ahci.c,
> declare it statically. This removes the last use
> of "inlined function with external linkage". See
> previous commit and commit 9de9fa5cf2 for rationale.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

thanks
-- PMM

Re: [PATCH-for-9.0? v2 3/4] qtest/libqos: Reduce size_to_prdtl() declaration scope

[Thomas Huth]

On 26/03/2024 18.10, Philippe Mathieu-Daudé wrote:
> Since size_to_prdtl() is only used within ahci.c,
> declare it statically. This removes the last use
> of "inlined function with external linkage". See
> previous commit and commit 9de9fa5cf2 for rationale.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---


Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH-for-9.0? v2 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20240313184954.42513-5-philmd@linaro.org>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index c9c3217ba4..f400f7d36c 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? v2 4/4] meson: Enable -Wstatic-in-inline

[Thomas Huth]

On 26/03/2024 18.10, 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

I think this is rather a compiler than a pre-processor flag, so 
s/CPPFLAG/CFLAGS/ ?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>


> inline function to be added in the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index c9c3217ba4..f400f7d36c 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',

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

[Philippe Mathieu-Daudé]

On 26/3/24 18:28, Thomas Huth wrote:
> On 26/03/2024 18.10, 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
> 
> I think this is rather a compiler than a pre-processor flag, so 
> s/CPPFLAG/CFLAGS/ ?

Oops indeed you are right, thanks!

> 
> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 
>> inline function to be added in the code base.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
>> ---
>>   meson.build | 1 +
>>   1 file changed, 1 insertion(+)

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

[Paolo Bonzini]

On 3/26/24 18:10, 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.

No problem with the first three patches, but -Wstatic-in-inline is not
warning for non-static inline functions.  The warning is enabled by
default by GCC (which has no way to disable it even) and by clang
outside headers:

f.h:
static int y;

inline int f()
{
     return y;
}

f.c:
#include "f.h"

int main()
{
}

$ clang f.c
./f.h:5:12: warning: static variable 'y' is used in an inline function with external linkage [-Wstatic-in-inline]

$ gcc f.c
f.h:5:12: warning: ‘y’ is static but used in inline function ‘f’ which is not static

The actual effect of this patch is to enable the warning on clang *even
outside headers* (clang only enables the warning in headers by default
because, if a static variable belongs to the main source file, it has a
single definition anyway unlike if it's defined in an included file).

For now I'm queuing patches 1-3 only.

Paolo

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index c9c3217ba4..f400f7d36c 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',

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

[Philippe Mathieu-Daudé]

On 27/3/24 10:26, Paolo Bonzini wrote:
> On 3/26/24 18:10, 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.
> 
> No problem with the first three patches, but -Wstatic-in-inline is not
> warning for non-static inline functions.  The warning is enabled by
> default by GCC (which has no way to disable it even) and by clang
> outside headers:
> 
> f.h:
> static int y;
> 
> inline int f()
> {
>      return y;
> }
> 
> f.c:
> #include "f.h"
> 
> int main()
> {
> }
> 
> $ clang f.c
> ./f.h:5:12: warning: static variable 'y' is used in an inline function 
> with external linkage [-Wstatic-in-inline]
> 
> $ gcc f.c
> f.h:5:12: warning: ‘y’ is static but used in inline function ‘f’ which 
> is not static
> 
> The actual effect of this patch is to enable the warning on clang *even
> outside headers* (clang only enables the warning in headers by default
> because, if a static variable belongs to the main source file, it has a
> single definition anyway unlike if it's defined in an included file).

IIUC your comment, you are worried about system headers declaring
non-static inline functions?

> 
> For now I'm queuing patches 1-3 only.
> 
> Paolo
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   meson.build | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/meson.build b/meson.build
>> index c9c3217ba4..f400f7d36c 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',
> 

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Il mer 27 mar 2024, 13:42 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> IIUC your comment, you are worried about system headers declaring
> non-static inline functions?
>

No, it's just that the flag (and thus the patch) is not doing what the
commit message says.

Perhaps you could instead add a checkpatch test that catches occurrences of
inline without static.

Paolo


> >
> > For now I'm queuing patches 1-3 only.
> >
> > Paolo
> >
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   meson.build | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index c9c3217ba4..f400f7d36c 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',
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2414 bytes --]