Implement CMake build system #647

Timmmm · 2024-12-18T12:02:51Z

This is a draft of a CMake build system. There are a couple of caveats which is why it is a draft:

Doesn't include RFVI, JSON docs, formal stuff, etc.
Commit is quite messy - includes moving files around and upgrading softfloat; I'll split that out in future.

However this should demonstrate the direction I think we should take. It has significant advantages:

IDE support is perfect, since it generates compile_commands.json. You can even go-to-definition to the generated code (I can demo this in the meeting if people want). Lots of IDEs also understand CMake and CTest.
Building all the emulators and running the tests is easy, parallel, and can generate an XML test result file (I haven't tested this though).
Better Windows support, e.g. CMake can generate Visual Studio project files.

There are several things that I think we should probably do before this which would make it a lot nicer:

Alasdair's new config system will hopefully turn xlen and flen into "initialisation time" settings so we don't need to build 4 different versions of the model & emulator.
The new project file - Modularise the specification #572 - means we don't need to list all the Sail files tediously in CMake.
When Implementing callbacks in sail-riscv for state-changing events #494 (state callbacks) is done, we can do Make RVFI a runtime option #642 and avoid having separate RVFI and non-RVFI models/emulators.

emulator/riscv_sim_RV64

CMakeLists.txt

cmake/modules/FindGMP.cmake

emulator/CMakeLists.txt

arichardson

Overall this is looking good!
I've left a few CMake comments, but it's quite hard to review with the softfloat upgrade included here.

Timmmm · 2025-01-02T15:03:12Z

Thanks for the review! I've split the other changes out into separate commits, so the CMake stuff is just the last commit now and hopefully easier to review.

I might drop the upgrade to softfloat actually since it didn't really change the code and hopefully we'll remove it at some point anyway in favour of the pure Sail implementation.

jordancarlin · 2025-01-02T16:28:31Z

I think it probably makes sense to skip the softfloat upgrade for the reasons you mentioned, but if we do decide to keep it I have some suggestions so that you don’t have to modify their Makefile.

arichardson

Regarding the commit that suggests bundling GMP - we could also use https://cmake.org/cmake/help/latest/module/ExternalProject.html#examples to build a known version statically (either opt-in or by default).

arichardson · 2025-01-02T17:04:33Z

Makefile

@@ -1,428 +0,0 @@
-# Select architecture: RV32 or RV64.


Could keep the makefile for the targets that aren't supported yet and just defer to CMake by default?
e.g.

csim: $(BUILD_DIR) cmake -S . -B $(BUILD_DIR) -GNinja -DCMAKE_BUILD_TYPE=Release && ninja -C $(BUILD_DIR)

Or to avoid needing ninja:

csim: $(BUILD_DIR) cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=Release && $(MAKE) -C $(BUILD_DIR)

arichardson

A few minor comments but generally looks good to me.

.gitignore

CMakeLists.txt

emulator/CMakeLists.txt

CMakeLists.txt

cmake/extra_fast.cmake

Timmmm · 2025-01-03T09:51:41Z

if we do decide to keep it I have some suggestions so that you don’t have to modify their Makefile.

We aren't actually using their Makefile at all with the CMake version; I just kept the modifications so that it compiled as a standalone commit. I'll just remove the upgrade anyway though.

@arichardson I think you put a comment somewhere that I can't find now about keeping the Makefile. I think I'll just try and implement all of the features currently in the Makefile before merging this, rather than have two build systems...

arichardson

Adding all the other targets may take quite a while, maybe easiest for now to add CMake support and remove targets from the Makefile as we port them over?

E.g. this PR just removes the csim target (or rather have it defer to CMake cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=Release && $(MAKE) -C $(BUILD_DIR)) and then later commits add the extra features? Otherwise I worry this will be a long standing feature branch that never gets merged.

jrtc27 · 2025-01-09T19:19:53Z

Adding all the other targets may take quite a while, maybe easiest for now to add CMake support and remove targets from the Makefile as we port them over?

E.g. this PR just removes the csim target (or rather have it defer to CMake cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=Release && $(MAKE) -C $(BUILD_DIR)) and then later commits add the extra features? Otherwise I worry this will be a long standing feature branch that never gets merged.

Doing CMake incrementally makes sense to me, but I'd rather not incrementally remove things from Makefile. It's annoying to have to use both depending on what you want to do. I don't really see the benefit of it. Just leave Makefile alone until it's entirely subsumed by CMakeLists.txt and then delete it.

arichardson · 2025-01-09T20:11:45Z

Adding all the other targets may take quite a while, maybe easiest for now to add CMake support and remove targets from the Makefile as we port them over?
E.g. this PR just removes the csim target (or rather have it defer to CMake cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=Release && $(MAKE) -C $(BUILD_DIR)) and then later commits add the extra features? Otherwise I worry this will be a long standing feature branch that never gets merged.

Doing CMake incrementally makes sense to me, but I'd rather not incrementally remove things from Makefile. It's annoying to have to use both depending on what you want to do. I don't really see the benefit of it. Just leave Makefile alone until it's entirely subsumed by CMakeLists.txt and then delete it.

That sounds even better to me

billmcspadden-riscv · 2025-01-09T23:11:55Z

I agree.

…

On Thu, Jan 9, 2025 at 2:12 PM Alexander Richardson < ***@***.***> wrote: Adding all the other targets may take quite a while, maybe easiest for now to add CMake support and remove targets from the Makefile as we port them over? E.g. this PR just removes the csim target (or rather have it defer to CMake cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=Release && $(MAKE) -C $(BUILD_DIR)) and then later commits add the extra features? Otherwise I worry this will be a long standing feature branch that never gets merged. Doing CMake incrementally makes sense to me, but I'd rather not incrementally remove things from Makefile. It's annoying to have to use both depending on what you want to do. I don't really see the benefit of it. Just leave Makefile alone until it's entirely subsumed by CMakeLists.txt and then delete it. That sounds even better to me — Reply to this email directly, view it on GitHub <#647 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOBMH4VMSUCF5IGWAX32J3JZ3AVCNFSM6AAAAABT2RMBV2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBRGE3DGMZRGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

Move the softfloat library to a dependencies directory. In future we can add GMP there too to reduce the number of external dependencies needed and to support static linking. The directory was renamed from `SoftFloat-3e` to `berkeley-softfloat-3` because that's how the latest version is named. It hasn't been upgraded in this commit to make it simpler to review and help Git track history. A future commit will upgrade it.

This is a draft of a CMake build system. It doesn't yet include RVFI, JSON docs, formal stuff, etc.

Timmmm · 2025-01-10T12:01:15Z

I've rebased it, removed the c_emulator rename and the softfloat update (we can do that later), and added RVFI support. I also updated the CI scripts.

I'm not sure I'm much of a fan of having two build systems during a transition. Verilator has exactly this - an old ./configure && make and an "experimental" CMake build system, and IMO it is quite confusing for users, and probably annoying for developers.

I reckon I can add the other targets without too much trouble, assuming they still work anyway! They aren't tested by CI and are presumably very rarely used.

arichardson · 2025-01-10T18:12:15Z

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 # See https://pre-commit.com for more information
 # See https://pre-commit.com/hooks.html for more hooks
-exclude: '^(prover_snapshots)|(generated_definitions)|(c_emulator/SoftFloat-3e)'
+exclude: '^(prover_snapshots)|(generated_definitions)|(dependencies/softfloat/berkeley-softfloat-3)'


I think we shouldn't run the checks on any dependencies? How about this:

Suggested change

exclude: '^(prover_snapshots)|(generated_definitions)|(dependencies/softfloat/berkeley-softfloat-3)'

exclude: '^(prover_snapshots)|(generated_definitions)|(dependencies)'

arichardson · 2025-01-10T18:12:55Z

Makefile

@@ -1,428 +0,0 @@
-# Select architecture: RV32 or RV64.
-ARCH ?= RV64


Need to keep makefile for now for the prover stuff?

arichardson · 2025-01-10T18:13:55Z

build_simulators.sh

-}
-
-test_build make ARCH=RV32 c_emulator/riscv_sim_RV32
-test_build make ARCH=RV64 c_emulator/riscv_sim_RV64


Is this documented anywhere as a sensible script to use? If so we could just update it to use CMake?

arichardson · 2025-01-10T18:17:57Z

Makefile

-ifneq (,$(COVERAGE))
-C_FLAGS += --coverage -O1
-SAIL_FLAGS += -Oconstant_fold
-else
-C_FLAGS += -O3 -flto=auto
-endif
-
-ifneq (,$(SAILCOV))
-ALL_BRANCHES = generated_definitions/c/all_branches
-C_FLAGS += -DSAILCOV
-SAIL_FLAGS += -c_coverage $(ALL_BRANCHES) -c_include sail_coverage.h
-C_LIBS += $(SAIL_LIB_DIR)/coverage/libsail_coverage.a -lm -lpthread -ldl
-endif
-
-# Optionally link C_LIBS statically. Unlike -static this will not
-# link glibc statically which is generally a bad idea.
-ifneq (,$(STATIC))
-    UNAME_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-    ifeq ($(UNAME_S),Darwin)
-        # Unfortunately the Mac linker does not support -Bstatic.
-        GMP_LIBS = $(shell pkg-config --variable=libdir gmp)/libgmp.a
-        C_LIBS_WRAPPED = $(C_LIBS)
-    else
-        C_LIBS_WRAPPED = -Wl,--push-state -Wl,-Bstatic $(C_LIBS) -Wl,--pop-state
-    endif
-else
-    C_LIBS_WRAPPED = $(C_LIBS)
-endif


I believe we have lost support for these options. I've used the sailcov option before, but not the other ones.

arichardson · 2025-01-10T18:19:50Z

Makefile

-riscv_hol: generated_definitions/hol4/$(ARCH)/riscvScript.sml
-riscv_hol_build: generated_definitions/hol4/$(ARCH)/riscvTheory.uo
-.PHONY: riscv_hol riscv_hol_build


Is the hol target still working? If so do we need to port it over to cmake?

arichardson · 2025-01-10T18:20:57Z

c_emulator/CMakeLists.txt

+    riscv_softfloat.h
+)
+
+foreach (arch IN ITEMS "rv32" "rv32d" "rv64" "rv64f" "rv32_rvfi" "rv32d_rvfi" "rv64_rvfi" "rv64f_rvfi")


Should we use variants here as well to make it easier to add the other targets in the future?

arichardson

Overall I still think this is a great change. A few things that were supported are no longer there.

I wonder if we should keep the Makefile for now and add something like the following to the top:

ifndef I_REALLY_NEED_TO_USE_MAKE
$(error The Makefile build system is deprecated, please use CMake instead. If you are using a target that does not exist there yet, please file and issue and re-run make with I_REALLY_NEED_TO_USE_MAKE=1 until this has been fixed)
endif

arichardson · 2025-01-10T18:24:33Z

I've rebased it, removed the c_emulator rename and the softfloat update (we can do that later), and added RVFI support. I also updated the CI scripts.

I'm not sure I'm much of a fan of having two build systems during a transition. Verilator has exactly this - an old ./configure && make and an "experimental" CMake build system, and IMO it is quite confusing for users, and probably annoying for developers.

I reckon I can add the other targets without too much trouble, assuming they still work anyway! They aren't tested by CI and are presumably very rarely used.

I agree that having multiple build systems is annoying and confusing. What do you think of my suggestion to add a (opt-out) error to the makefile to point people towards CMake.

We should probably also update the README.

jrtc27 · 2025-01-10T18:25:15Z

CMakeLists.txt

+    message(FATAL_ERROR "Sail not found. See README.md for installation instructions.")
+endif()
+
+set(DEFAULT_ARCHITECTURES "rv32;rv64" CACHE STRING "Architectures to build by default (rv32|rv64|rv32d|rv64f)(_rvfi)? " )


It's not clear to me on first read what these things mean. My initial interpretation was:
rv32: no float
rv64: no float
rv32d: FLEN=64
rv64f: FLEN=32

But then that's odd because FLEN=64 is a sensible configuration for RV64 (more so than FLEN=32, even), and of course that's not actually what's going on, there's a default non-zero FLEN and then the suffixed ones are overrides. But those names are confusing and weird.

On top of that, this is a behavioural change. The current Makefile build always defaults to FLEN=64 even for RV32, but you've changed that default to be FLEN=32.

arichardson · 2025-01-10T18:25:24Z

Once this lands, I am happy to submit some follow-up improvements to support static builds and builds that build GMP from source as part of the build process to avoid any system dependencies.

jrtc27 · 2025-01-10T18:28:30Z

I've rebased it, removed the c_emulator rename and the softfloat update (we can do that later), and added RVFI support. I also updated the CI scripts.
I'm not sure I'm much of a fan of having two build systems during a transition. Verilator has exactly this - an old ./configure && make and an "experimental" CMake build system, and IMO it is quite confusing for users, and probably annoying for developers.
I reckon I can add the other targets without too much trouble, assuming they still work anyway! They aren't tested by CI and are presumably very rarely used.

I agree that having multiple build systems is annoying and confusing. What do you think of my suggestion to add a (opt-out) error to the makefile to point people towards CMake.

We should probably also update the README.

I mean, if the Makefile is needed in some cases then it shouldn't be an error to use it. And if it's not needed then it can be deleted.

It's not confusing at all to have both, provided the CMake one is clearly labelled as experimental and in-progress. What's confusing is when there are two documented ways of building (or one documented but the other one isn't clearly labelled as "don't use yet unless you really want to").

arichardson · 2025-01-10T18:30:49Z

I've rebased it, removed the c_emulator rename and the softfloat update (we can do that later), and added RVFI support. I also updated the CI scripts.
I'm not sure I'm much of a fan of having two build systems during a transition. Verilator has exactly this - an old ./configure && make and an "experimental" CMake build system, and IMO it is quite confusing for users, and probably annoying for developers.
I reckon I can add the other targets without too much trouble, assuming they still work anyway! They aren't tested by CI and are presumably very rarely used.

I agree that having multiple build systems is annoying and confusing. What do you think of my suggestion to add a (opt-out) error to the makefile to point people towards CMake.
We should probably also update the README.

I mean, if the Makefile is needed in some cases then it shouldn't be an error to use it. And if it's not needed then it can be deleted.

It's not confusing at all to have both, provided the CMake one is clearly labelled as experimental and in-progress. What's confusing is when there are two documented ways of building (or one documented but the other one isn't clearly labelled as "don't use yet unless you really want to").

I agree it shouldn't be an error when it's needed. I was just suggesting making it an error that you can override to find out if anyone is using the targets that don't exist in the CMkae world yet.

If we just add a deprecation warning or keep the makefile around unchanged no one is going to report it until it becomes an error.

Timmmm mentioned this pull request Dec 18, 2024

Switch to CMake #551

Open