| Message ID | 20230117135203.3049709-4-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings | expand |
On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote: > From: Kevin Wolf <kwolf@redhat.com> > > This enables clang's thread safety analysis (TSA), which we'll use to > statically check the block graph locking. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20221207131838.239125-9-kwolf@redhat.com> > Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > 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
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 <kwolf@redhat.com> >> >> This enables clang's thread safety analysis (TSA), which we'll use to >> statically check the block graph locking. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> Message-Id: <20221207131838.239125-9-kwolf@redhat.com> >> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> 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
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 <kwolf@redhat.com> > >> > >> This enables clang's thread safety analysis (TSA), which we'll use to > >> statically check the block graph locking. > >> > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> Message-Id: <20221207131838.239125-9-kwolf@redhat.com> > >> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> --- > >> 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
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 <kwolf@redhat.com> > > >> > > >> This enables clang's thread safety analysis (TSA), which we'll use to > > >> statically check the block graph locking. > > >> > > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > >> Message-Id: <20221207131838.239125-9-kwolf@redhat.com> > > >> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > >> --- > > >> 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
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