Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linux ci - Compile KVM with goto-cc #5916

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Conversation

nmanthey
Copy link
Contributor

This change adds a github workflow that compiles parts of the Linux kernel with goto-cc, and fails in case compilation fails. Furthermore, the input fails that lead to the failure are archived, to allow easy consumption and investigation.

Note: This is currently failing, and should help to improve goto-cc to be able to compile Linux

An example failed run with the broken input as artifact has been created as part of my testing: https://github.com/nmanthey/cbmc/pull/5/checks?check_run_id=2093209018

  • Each commit message has a non-empty body, explaining why the change was made.
  • [N/A] Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • [N/A] The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • [N/A] Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • [N/A] My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@nmanthey nmanthey requested a review from a team as a code owner March 12, 2021 08:46
@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #5916 (1fbfd5e) into develop (7663a57) will decrease coverage by 0.02%.
The diff coverage is 58.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5916      +/-   ##
===========================================
- Coverage    74.30%   74.27%   -0.03%     
===========================================
  Files         1444     1444              
  Lines       157453   157460       +7     
===========================================
- Hits        116995   116953      -42     
- Misses       40458    40507      +49     
Impacted Files Coverage Δ
src/solvers/smt2/smt2_conv.h 100.00% <ø> (ø)
src/ansi-c/c_typecheck_initializer.cpp 61.61% <44.44%> (-12.97%) ⬇️
src/solvers/smt2/smt2_conv.cpp 60.38% <52.94%> (ø)
src/solvers/smt2/smt2_solver.cpp 83.11% <65.51%> (ø)
src/util/config.cpp 55.42% <0.00%> (+3.03%) ⬆️
src/util/config.h 62.50% <0.00%> (+12.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fdf995...1fbfd5e. Read the comment docs.

Comment on lines +73 to +53
- uses: actions/upload-artifact@v2
with:
name: CPROVER-faultyInput
path: CPROVER/faultyInput/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does one actually download these? GitHub keeps telling me that I need to have the "actions" scope, perhaps the repository permissions are set up in a way that doesn't permit non-members to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about doing tail -n +1 CPROVER/faultyInput/* instead? Really simple and avoids any permissions problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the checks page (here, e.g.: https://github.com/diffblue/cbmc/pull/5916/checks there is an "artifacts" button on the top left, which allows (me) to download, I did not see any permission restrictions in the description of the action). Yes, I could also dump the content of the files, but that likely floods the output, as those are preprocessed already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, thank you, that was too easy. A first fix is in #5928.

Comment on lines 21 to 25
sudo apt-get install --no-install-recommends -y coreutils build-essential gcc git make flex bison software-properties-common curl python
sudo apt-get install --no-install-recommends -y bin86 gdb bcc liblzma-dev python-dev gettext iasl uuid-dev libncurses5-dev libncursesw5-dev pkg-config
sudo apt-get install --no-install-recommends -y libgtk2.0-dev libyajl-dev sudo time ccache
sudo apt-get install --no-install-recommends -y automake bc libssl-dev dkms libelf-dev libudev-dev libpci-dev libiberty-dev autoconf gawk jq
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I recall, we just need build-essential libssl-dev for the Linux kernel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, that depends on the configuration. I can minimize this set.

Comment on lines 50 to 52
- name: Get Linux v5.10
run: git clone -b v5.10 --depth=1 https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git linux_5_10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to always use the latest release (at the risk of builds surprisingly breaking) and perhaps also using tar/zip instead of cloning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people are willing to take the risk, and address the issue (or roll this action back to the last known working release) I can build logic to always access the latest release.

On the other hand, I would like to be able to ensure that a specific version is always working.

This is the risk I see when always using the latest release: Once the latest release fails, people might ignore the failure or disable this action to unblock themselves. Afterwards, even compiling older relases might start to break again. Hence, I decided to pick a specific release for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have two such actions? One with a fixed, longterm-maintenance release (5.10 seems like a reasonable candidate) and one using the "master" branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do the stable one for now, and iterate once that one is in.

@nmanthey nmanthey force-pushed the linux-ci branch 2 times, most recently from 95c617f to e5d5a1e Compare March 15, 2021 13:29
@tautschnig
Copy link
Collaborator

We will next need #5730 to be merged.

@tautschnig
Copy link
Collaborator

We will next need #5730 to be merged.

And then #5723, at which point the job should actually succeed.

@tautschnig
Copy link
Collaborator

We will next need #5730 to be merged.

And then #5723, at which point the job should actually succeed.

Both of which are now merged, I believe this test should now succeed.

Comment on lines 63 to 65
make -C linux_5_10 allnoconfig
make -C linux_5_10 kvm_guest.config
make -C linux_5_10 bzImage -j $(nproc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest the following variation of this:

cat > kvm-config <<EOF
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_MULTIUSER=y
CONFIG_NET=y
CONFIG_VIRTUALIZATION=y
CONFIG_HYPERVISOR_GUEST=y
CONFIG_PARAVIRT=y
CONFIG_KVM_GUEST=y
CONFIG_KVM=y
CONFIG_KVM_INTEL=y
CONFIG_KVM_AMD=y
EOF
KCONFIG_ALLCONFIG=kvm-config make allnoconfig
make bzImage -j $(nproc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can re-iterate

make -C linux_5_10 kvm_guest.config
make -C linux_5_10 bzImage -j $(nproc)
du -h . --max-depth=1
rm "linux_5_10/arch/x86/kernel/kvm.o"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above, perhaps instead do:

rm arch/x86/kernel/kvm.o
find arch/x86/kvm/ -name "*.o" -delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will iterate

make -C linux_5_10 bzImage -j $(nproc)
du -h . --max-depth=1
rm "linux_5_10/arch/x86/kernel/kvm.o"
one-line-scan/one-line-scan --add-to-path $(pwd)/src/cbmc --add-to-path $(pwd)/src/goto-diff --add-to-path $(pwd)/src/goto-cc --no-analysis --trunc-existing --extra-cflags -Wno-error -o CPROVER -j 3 -- make -C linux_5_10 bzImage -j $(nproc) -k || true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we we should be ignoring failures here, thus drop -k || true. Also, shouldn't one-line-scan use (at least) the same -j as the command being run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should work.

The '-j' for one-line-scan is limited to not consume too much memory. For github, that should be good enough. For other use cases, that might be higher.

@nmanthey
Copy link
Contributor Author

I addressed the comments, and factored out the actual compilation into a separate script, so that these steps can be executed in other environments more easily.

@nmanthey nmanthey removed their assignment Mar 22, 2021
@tautschnig
Copy link
Collaborator

I addressed the comments, and factored out the actual compilation into a separate script, so that these steps can be executed in other environments more easily.

Thanks a lot, I'll review whether I already have a patch for the invariant failure.

@tautschnig
Copy link
Collaborator

I addressed the comments, and factored out the actual compilation into a separate script, so that these steps can be executed in other environments more easily.

Thanks a lot, I'll review whether I already have a patch for the invariant failure.

A proposal for the patch is in #5967, but I need to reconsider whether that's the correct approach (it is borrowed from #5738, which also made a number of other changes).

@tautschnig tautschnig self-assigned this Mar 22, 2021
Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me (modulo the CI failure).

I will wait for this to be fixed then I will approve this to get in.

@tautschnig tautschnig changed the title Linux ci - Compile KVM with goto-cc Linux ci - Compile KVM with goto-cc [depends-on: #5967] Mar 31, 2021
@nmanthey nmanthey force-pushed the linux-ci branch 2 times, most recently from 7931bda to 21abee1 Compare April 19, 2021 08:22
@tautschnig tautschnig changed the title Linux ci - Compile KVM with goto-cc [depends-on: #5967] Linux ci - Compile KVM with goto-cc Apr 21, 2021
@tautschnig
Copy link
Collaborator

@nmanthey #5967 is now merged, I believe this should pass once rebased.

Script the steps required to build linux, so that they can also be
executed locally.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Similarly to Xen, CBMC should be able to compile the linux kernel. As
compiling the full kernel is very time consuming, and with CBMC also
space consuming, only compile a core part all its dependencies, with
the smalles configuration possible. For this core part, we select the
KVM hypervisor.

Once this is working, the configuration, as well as targets to be
compiled can be increased.

When the linuxci fails, we want to be able to easily understand the
failure. The used one-line-scan tool already captures input files that
cannot be handled by goto-cc. Hence, also archives these files for easy
access in the web UI.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
on:
pull_request:
branches:
- '**'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant about (another) CI job that runs on all PRs, specially one that we expect to report failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to suggest another branch pattern. I can also target the develop and production branch only. That might result in late surprises.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TGWDB Why would you expect failures here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nmanthey I'm no git expert and don't have a better suggestion. I agree that late failures are not a good idea.

@tautschnig Based on the original PR text:

"This change adds a github workflow that compiles parts of the Linux kernel with goto-cc, and fails in case compilation fails. Furthermore, the input fails that lead to the failure are archived, to allow easy consumption and investigation."

I'm happy if this works without failures, but was (mis?)reading that we expect this to fail more often due to the target and perhaps related aspects of cbmc.

If we expect goto-cc to never fail this except for when we break something then I have no objection.

@tautschnig tautschnig merged commit dc30727 into diffblue:develop Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants