[PATCH 0/2] gitlab-ci.yml: Add jobs to test CFI

[PATCH 0/2] gitlab-ci.yml: Add jobs to test CFI

[Daniele Buono]

For a few months now QEMU has had options to enable compiler-based
control-flow integrity if built with clang.

While this feature has a low maintenance, It's probably still better to
add tests to the CI environment to check that an update doesn't break it.

As an added benefit, this also inherently tests LTO. The patch allow
gitlab testing of:
* --enable-cfi: forward-edge cfi (function pointers)
* --enable-safe-stack: backward-edge cfi (return pointers)

My original intention was to create a single chain of
build -> check -> acceptance, with all the targets compiled by default.
Unfortunately, the resulting artifact is too big and won't be uploaded.

So I split the test in two chains, that should cover all non-deprecated
targets as of today.

I also had to add a small patch to allow a custom selection for make
parallelism. This is because the gitlab runner nodes only have ~3.5GB of
ram, and with the default parallelism (2), in some cases two ld
instances will start working on two binaries and exaust the memory.
By only forcing one make job at a time, this is avoided.

Test runs of the full pipeline are here (cfi-ci branch):
https://gitlab.com/dbuono/qemu/-/pipelines/259931154

Daniele Buono (2):
  gitlab-ci.yml: Allow custom make parallelism
  gitlab-ci.yml: Add jobs to test CFI flags

 .gitlab-ci.yml | 94 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

-- 
2.30.0

[PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Daniele Buono]

Currently, make parallelism at build time is defined as #cpus+1.
Some build jobs may need (or benefit from) a different number.

An example is builds with LTO where, because of the huge demand
of memory at link time, gitlab runners fails if two linkers are
run concurrently

This patch retains the default value of #cpus+1 but allows setting
the "JOBS" variable to a different number when applying the template

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8b6d495288..5c198f05d4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -17,7 +17,7 @@ include:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
   before_script:
-    - JOBS=$(expr $(nproc) + 1)
+    - JOBS=${JOBS:-$(expr $(nproc) + 1)}
   script:
     - mkdir build
     - cd build
-- 
2.30.0

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Paolo Bonzini]

On 23/02/21 00:01, Daniele Buono wrote:
> Currently, make parallelism at build time is defined as #cpus+1. Some 
> build jobs may need (or benefit from) a different number. An example is 
> builds with LTO where, because of the huge demand of memory at link 
> time, gitlab runners fails if two linkers are run concurrently This 
> patch retains the default value of #cpus+1 but allows setting the "JOBS" 
> variable to a different number when applying the template

As I just found out, you can add -Dbackend_max_links=1 to the meson 
command line instead if LTO is enabled.

Paolo

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Daniele Buono]

This works, but setting this value to 1 for everybody seems a bit too
restrictive. While the gitlab ci runners don't have enough memory for
this, that's not necessarily true for every build platform, and linking
multiple targets in parallel with LTO can result in a big save in time,
so I'd prefer a customizable way.

How about adding a flag `--max-ld-procs` to configure to manually set
backend_max_links?

This would also allow setting it up to any specific number above 1,
which looking at the Makefile seems to not be possible now: because of
how the -j flag is passed from make to ninja, a compilation is either
sequential or parallel based on #cpus

On 2/23/2021 3:12 AM, Paolo Bonzini wrote:
> On 23/02/21 00:01, Daniele Buono wrote:
>> Currently, make parallelism at build time is defined as #cpus+1. Some 
>> build jobs may need (or benefit from) a different number. An example 
>> is builds with LTO where, because of the huge demand of memory at link 
>> time, gitlab runners fails if two linkers are run concurrently This 
>> patch retains the default value of #cpus+1 but allows setting the 
>> "JOBS" variable to a different number when applying the template
> 
> As I just found out, you can add -Dbackend_max_links=1 to the meson 
> command line instead if LTO is enabled.
> 
> Paolo
> 

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Paolo Bonzini]

On 23/02/21 20:34, Daniele Buono wrote:
> This works, but setting this value to 1 for everybody seems a bit too
> restrictive. While the gitlab ci runners don't have enough memory for
> this, that's not necessarily true for every build platform, and linking
> multiple targets in parallel with LTO can result in a big save in time,
> so I'd prefer a customizable way.
> 
> How about adding a flag `--max-ld-procs` to configure to manually set
> backend_max_links?

Another possibility is to invoke "meson configure build 
-Dbackend_max_links=1" after configure.

Paolo

> This would also allow setting it up to any specific number above 1,
> which looking at the Makefile seems to not be possible now: because of
> how the -j flag is passed from make to ninja, a compilation is either
> sequential or parallel based on #cpus

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Daniele Buono]

On 2/24/2021 2:44 AM, Paolo Bonzini wrote:
> On 23/02/21 20:34, Daniele Buono wrote:
>> This works, but setting this value to 1 for everybody seems a bit too
>> restrictive. While the gitlab ci runners don't have enough memory for
>> this, that's not necessarily true for every build platform, and linking
>> multiple targets in parallel with LTO can result in a big save in time,
>> so I'd prefer a customizable way.
>>
>> How about adding a flag `--max-ld-procs` to configure to manually set
>> backend_max_links?
> 
> Another possibility is to invoke "meson configure build 
> -Dbackend_max_links=1" after configure.

I like this, I'll send a v2 soon where I replace this patch with one
just for linking.

Daniele

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Wainer dos Santos Moschetta]

On 2/22/21 8:01 PM, Daniele Buono wrote:
> Currently, make parallelism at build time is defined as #cpus+1.
> Some build jobs may need (or benefit from) a different number.
>
> An example is builds with LTO where, because of the huge demand
> of memory at link time, gitlab runners fails if two linkers are
> run concurrently
>
> This patch retains the default value of #cpus+1 but allows setting
> the "JOBS" variable to a different number when applying the template
>
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>   .gitlab-ci.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 8b6d495288..5c198f05d4 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -17,7 +17,7 @@ include:
>     stage: build
>     image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
>     before_script:
> -    - JOBS=$(expr $(nproc) + 1)
> +    - JOBS=${JOBS:-$(expr $(nproc) + 1)}
>     script:
>       - mkdir build
>       - cd build

[PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags

[Daniele Buono]

QEMU has had options to enable control-flow integrity features
for a few months now. Add two sets of build/check/acceptance
jobs to ensure the binary produced is working fine.

The two sets allow testing of x86_64 binaries for every target
that is not deprecated.

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 .gitlab-ci.yml | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 5c198f05d4..f2fea8e2eb 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -479,6 +479,98 @@ clang-user:
       --extra-cflags=-fsanitize=undefined --extra-cflags=-fno-sanitize-recover=undefined
     MAKE_CHECK_ARGS: check-unit check-tcg
 
+# Set JOBS=1 because this requires LTO and ld consumes a large amount of memory.
+# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds concurrently
+# and triggers an Out-Of-Memory error
+#
+# Because of how slirp is used in QEMU, we need to have CFI also on libslirp.
+# System-wide version in fedora is not compiled with CFI so we recompile it using
+# -enable-slirp=git
+#
+# Split in two sets of build/check/acceptance because a single build job for every
+# target creates an artifact archive too big to be uploaded
+build-cfi-set1:
+  <<: *native_build_job_definition
+  needs:
+  - job: amd64-fedora-container
+  variables:
+    JOBS: 1
+    AR: llvm-ar
+    IMAGE: fedora
+    CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug
+      --enable-safe-stack --enable-slirp=git
+    TARGETS: aarch64-softmmu arm-softmmu alpha-softmmu i386-softmmu ppc-softmmu
+      ppc64-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu sparc-softmmu
+      sparc64-softmmu x86_64-softmmu
+      aarch64-linux-user aarch64_be-linux-user arm-linux-user i386-linux-user
+      ppc64-linux-user ppc64le-linux-user s390x-linux-user x86_64-linux-user
+    MAKE_CHECK_ARGS: check-build
+  timeout: 3h
+  artifacts:
+    expire_in: 2 days
+    paths:
+      - build
+
+check-cfi-set1:
+  <<: *native_test_job_definition
+  needs:
+    - job: build-cfi-set1
+      artifacts: true
+  variables:
+    IMAGE: fedora
+    MAKE_CHECK_ARGS: check
+
+acceptance-cfi-set1:
+  <<: *native_test_job_definition
+  needs:
+    - job: build-cfi-set1
+      artifacts: true
+  variables:
+    IMAGE: fedora
+    MAKE_CHECK_ARGS: check-acceptance
+  <<: *acceptance_definition
+
+build-cfi-set2:
+  <<: *native_build_job_definition
+  needs:
+  - job: amd64-fedora-container
+  variables:
+    JOBS: 1
+    AR: llvm-ar
+    IMAGE: fedora
+    CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug
+      --enable-safe-stack --enable-slirp=git
+    TARGETS: avr-softmmu cris-softmmu hppa-softmmu m68k-softmmu
+      microblaze-softmmu microblazeel-softmmu mips-softmmu mips64-softmmu
+      mips64el-softmmu mipsel-softmmu moxie-softmmu nios2-softmmu or1k-softmmu
+      rx-softmmu sh4-softmmu sh4eb-softmmu tricore-softmmu xtensa-softmmu
+      xtensaeb-softmmu
+    MAKE_CHECK_ARGS: check-build
+  timeout: 3h
+  artifacts:
+    expire_in: 2 days
+    paths:
+      - build
+
+check-cfi-set2:
+  <<: *native_test_job_definition
+  needs:
+    - job: build-cfi-set2
+      artifacts: true
+  variables:
+    IMAGE: fedora
+    MAKE_CHECK_ARGS: check
+
+acceptance-cfi-set2:
+  <<: *native_test_job_definition
+  needs:
+    - job: build-cfi-set2
+      artifacts: true
+  variables:
+    IMAGE: fedora
+    MAKE_CHECK_ARGS: check-acceptance
+  <<: *acceptance_definition
+
 tsan-build:
   <<: *native_build_job_definition
   variables:
-- 
2.30.0

Re: [PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags

[Paolo Bonzini]

On 23/02/21 00:01, Daniele Buono wrote:
> +# Set JOBS=1 because this requires LTO and ld consumes a large amount of memory.
> +# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds concurrently
> +# and triggers an Out-Of-Memory error

Does it make sense to test only one target instead?

> +# Because of how slirp is used in QEMU, we need to have CFI also on libslirp.
> +# System-wide version in fedora is not compiled with CFI so we recompile it using
> +# -enable-slirp=git

Can you explain what you mean, and perhaps add a check or warning for 
incompatible settings?

Paolo

Re: [PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags

[Daniele Buono]

On 2/23/2021 3:11 AM, Paolo Bonzini wrote:
> On 23/02/21 00:01, Daniele Buono wrote:
>> +# Set JOBS=1 because this requires LTO and ld consumes a large amount 
>> of memory.
>> +# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds 
>> concurrently
>> +# and triggers an Out-Of-Memory error
> 
> Does it make sense to test only one target instead?

I'd prefer grouping multiple targets per job so that the number of jobs 
doesn't explode, and stopping ninja from linking in parallel does solve 
the issue.

There's also the issue that tests are also compiled here so you may end
up with two linkers anyway. However the chance that this will end up in
an out-of-memory error is quite smaller (possibly zero) since tests
don't link that many object files together.

> 
>> +# Because of how slirp is used in QEMU, we need to have CFI also on 
>> libslirp.
>> +# System-wide version in fedora is not compiled with CFI so we 
>> recompile it using
>> +# -enable-slirp=git
> 
> Can you explain what you mean, and perhaps add a check or warning for 
> incompatible settings?

Certainly. The issue here is that there is a function in libslirp that
is used as callbacks for QEMU Timers: ra_timer_handler
(There may be others, but of this one I'm sure because I traced it).

This is not an issue when you compile slirp with qemu, since the whole
library now has CFI informations and is statically linked in the QEMU
binary. It becomes an issue if you are dynamically linking a system-wide
libslirp, as it happens on Fedora.

I'd be happy to add a check on configure/meson that ends the configure
step with an error when this happens, but that would technically be an
independent patch that I'd work on in parallel to this one.
I would prefer to not automatically select the git-based libslirp
because that may go unnoticed when configuring.

> 
> Paolo
> 

Re: [PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags

[Paolo Bonzini]

On 24/02/21 18:55, Daniele Buono wrote:
>>
>> Does it make sense to test only one target instead?
> 
> I'd prefer grouping multiple targets per job so that the number of jobs doesn't explode, and stopping ninja from linking in parallel does solve the issue.

Yeah, backend_max_links should do it.  The 3 hour timeout scared me.

>> Can you explain what you mean, and perhaps add a check or warning for 
>> incompatible settings?
> 
> Certainly. The issue here is that there is a function in libslirp that
> is used as callbacks for QEMU Timers: ra_timer_handler
> (There may be others, but of this one I'm sure because I traced it).
> 
> This is not an issue when you compile slirp with qemu, since the whole
> library now has CFI informations and is statically linked in the QEMU
> binary. It becomes an issue if you are dynamically linking a system-wide
> libslirp, as it happens on Fedora.
> 
> I'd be happy to add a check on configure/meson that ends the configure
> step with an error when this happens, but that would technically be an
> independent patch that I'd work on in parallel to this one.
> I would prefer to not automatically select the git-based libslirp
> because that may go unnoticed when configuring.

Sounds good.  For now just add a comment, please.

Paolo