[3/3] configure: Enable -Wthread-safety if present

Commit Message

Emanuele Giuseppe Esposito Jan. 17, 2023, 1:52 p.m. UTC

From: Kevin Wolf <[e-mail]>

This enables clang's thread safety analysis (TSA), which we'll use to
statically check the block graph locking.

Signed-off-by: Kevin Wolf <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Emanuele Giuseppe Esposito <[e-mail]>
Signed-off-by: Kevin Wolf <[e-mail]>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

Comments

Daniel P. Berrangé Jan. 17, 2023, 2:02 p.m. UTC | #1

On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> From: Kevin Wolf <[e-mail]>
> 
> This enables clang's thread safety analysis (TSA), which we'll use to
> statically check the block graph locking.
> 
> Signed-off-by: Kevin Wolf <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Emanuele Giuseppe Esposito <[e-mail]>
> Signed-off-by: Kevin Wolf <[e-mail]>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index 2281892657..14668e6269 100755
> --- a/configure
> +++ b/configure
> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
>  add_to warn_flags -Wendif-labels
>  add_to warn_flags -Wexpansion-to-defined
>  add_to warn_flags -Wimplicit-fallthrough=2
> +add_to warn_flags -Wthread-safety

Does this thread safety analysis have any kind of measurable
impact on compilation speed ?

Our CI jobs are quite sensitive to any increase in build
time.


With regards,
Daniel

Emanuele Giuseppe Esposito Jan. 17, 2023, 2:41 p.m. UTC | #2

Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
>> From: Kevin Wolf <[e-mail]>
>>
>> This enables clang's thread safety analysis (TSA), which we'll use to
>> statically check the block graph locking.
>>
>> Signed-off-by: Kevin Wolf <[e-mail]>
>> Message-Id: <[e-mail]>
>> Reviewed-by: Emanuele Giuseppe Esposito <[e-mail]>
>> Signed-off-by: Kevin Wolf <[e-mail]>
>> ---
>>  configure | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/configure b/configure
>> index 2281892657..14668e6269 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
>>  add_to warn_flags -Wendif-labels
>>  add_to warn_flags -Wexpansion-to-defined
>>  add_to warn_flags -Wimplicit-fallthrough=2
>> +add_to warn_flags -Wthread-safety
> 
> Does this thread safety analysis have any kind of measurable
> impact on compilation speed ?
> 
> Our CI jobs are quite sensitive to any increase in build
> time.

From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):

without clang:
real    2m46.729s
user    19m24.122s
sys     2m58.643s

with clang:
real    2m45.204s
user    19m52.096s
sys     2m9.036s

So there should be no significative impact.

I also forgot to mention that this serie fixes the CI failure seen in:

https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
https://gitlab.com/qemu-project/qemu/-/jobs/3479763746

Thank you,
Emanuele
> 
> 
> With regards,
> Daniel

Daniel P. Berrangé Jan. 17, 2023, 3:01 p.m. UTC | #3

On Tue, Jan 17, 2023 at 03:41:29PM +0100, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> > On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> >> From: Kevin Wolf <[e-mail]>
> >>
> >> This enables clang's thread safety analysis (TSA), which we'll use to
> >> statically check the block graph locking.
> >>
> >> Signed-off-by: Kevin Wolf <[e-mail]>
> >> Message-Id: <[e-mail]>
> >> Reviewed-by: Emanuele Giuseppe Esposito <[e-mail]>
> >> Signed-off-by: Kevin Wolf <[e-mail]>
> >> ---
> >>  configure | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/configure b/configure
> >> index 2281892657..14668e6269 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
> >>  add_to warn_flags -Wendif-labels
> >>  add_to warn_flags -Wexpansion-to-defined
> >>  add_to warn_flags -Wimplicit-fallthrough=2
> >> +add_to warn_flags -Wthread-safety
> > 
> > Does this thread safety analysis have any kind of measurable
> > impact on compilation speed ?
> > 
> > Our CI jobs are quite sensitive to any increase in build
> > time.
> 
> From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
> Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):
> 
> without clang:
> real    2m46.729s
> user    19m24.122s
> sys     2m58.643s
> 
> with clang:
> real    2m45.204s
> user    19m52.096s
> sys     2m9.036s
> 
> So there should be no significative impact.
> 
> I also forgot to mention that this serie fixes the CI failure seen in:
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
> https://gitlab.com/qemu-project/qemu/-/jobs/3479763746

Odd, that job already has  -Wthread-safety included in CFLAGS, which
would seem to make this patch redundant, but I don't see where
-Wthread-safety came from in that pipeline 


With regards,
Daniel

Kevin Wolf Jan. 17, 2023, 3:59 p.m. UTC | #4

Am 17.01.2023 um 16:01 hat Daniel P. Berrangé geschrieben:
> On Tue, Jan 17, 2023 at 03:41:29PM +0100, Emanuele Giuseppe Esposito wrote:
> > 
> > 
> > Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> > > On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> > >> From: Kevin Wolf <[e-mail]>
> > >>
> > >> This enables clang's thread safety analysis (TSA), which we'll use to
> > >> statically check the block graph locking.
> > >>
> > >> Signed-off-by: Kevin Wolf <[e-mail]>
> > >> Message-Id: <[e-mail]>
> > >> Reviewed-by: Emanuele Giuseppe Esposito <[e-mail]>
> > >> Signed-off-by: Kevin Wolf <[e-mail]>
> > >> ---
> > >>  configure | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/configure b/configure
> > >> index 2281892657..14668e6269 100755
> > >> --- a/configure
> > >> +++ b/configure
> > >> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
> > >>  add_to warn_flags -Wendif-labels
> > >>  add_to warn_flags -Wexpansion-to-defined
> > >>  add_to warn_flags -Wimplicit-fallthrough=2
> > >> +add_to warn_flags -Wthread-safety
> > > 
> > > Does this thread safety analysis have any kind of measurable
> > > impact on compilation speed ?
> > > 
> > > Our CI jobs are quite sensitive to any increase in build
> > > time.
> > 
> > From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
> > Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):
> > 
> > without clang:
> > real    2m46.729s
> > user    19m24.122s
> > sys     2m58.643s
> > 
> > with clang:
> > real    2m45.204s
> > user    19m52.096s
> > sys     2m9.036s
> > 
> > So there should be no significative impact.
> > 
> > I also forgot to mention that this serie fixes the CI failure seen in:
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
> > https://gitlab.com/qemu-project/qemu/-/jobs/3479763746
> 
> Odd, that job already has  -Wthread-safety included in CFLAGS, which
> would seem to make this patch redundant, but I don't see where
> -Wthread-safety came from in that pipeline 

This patch is what was already in the branch in which the above CI tests
ran. I dropped it from the pull request because of these build failures
on FreeBSD. (Their libc includes TSA annotations, which means that QEMU
functions have to be annotated as well when they call pthread locking
functions, before we can enable -Wthread-safety.)

So patches 1 and 2 fix the failure that you would otherwise see with
this patch.

Kevin

Patch

diff --git a/configure b/configure
index 2281892657..14668e6269 100755
--- a/configure
+++ b/configure
@@ -1183,6 +1183,7 @@  add_to warn_flags -Wnested-externs
 add_to warn_flags -Wendif-labels
 add_to warn_flags -Wexpansion-to-defined
 add_to warn_flags -Wimplicit-fallthrough=2
+add_to warn_flags -Wthread-safety
 
 nowarn_flags=
 add_to nowarn_flags -Wno-initializer-overrides