Skip to content

Refine the build system #203

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DrXiao
Copy link
Collaborator

@DrXiao DrXiao commented May 18, 2025

This pull request resolves the following issues:

Fix empty TARGET_EXEC variable

Previously, I noticed that the build system directly executed the stage1 compiler to build the stage2 compiler, instead of invoking qemu-user. This issue could be observed by running make VERBOSE=1

$ make VERBOSE=1
...
...
+out/shecc-stage1.elf -o out/shecc-stage2.elf src/main.c
...

That is, TARGET_EXEC, which stores the the name of an architecture-specific qemu executable, is left empty and not properly assigned.

Therefore, these changes fix it to ensure the build system invokes qemu as expected.

Close #44

Summary by Bito

This pull request refines the build system by fixing the empty TARGET_EXEC variable issue, enhancing Makefiles for ARM and RISC-V architectures, and adding prerequisite checks to improve build reliability.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2 - The changes are straightforward and primarily focus on build system enhancements, making them easy to review.

@jserv jserv mentioned this pull request May 18, 2025
@jserv
Copy link
Collaborator

jserv commented May 18, 2025

If target architecture does not match host (e.g., configuring Arm32 on x86-64), the build system should warn and stop at stage 0.

@DrXiao
Copy link
Collaborator Author

DrXiao commented May 18, 2025

If target architecture does not match host (e.g., configuring Arm32 on x86-64), the build system should warn and stop at stage 0.

I am confused by this description and not sure if I understood correctly. If we configure Arm32 on x86-64, the build system should proceed with bootstrapping. The stage 2 compiler would be built by the stage 1 compiler running through qemu-arm, rather than stopping the build.

@jserv
Copy link
Collaborator

jserv commented May 18, 2025

If we configure Arm32 on x86-64, the build system should proceed with bootstrapping.

Yes, it is valid when qemu-arm or qemu-riscv32 emulators are properly configured in the environment. However, the current implementation lacks comprehensive documentation or messages that would explain the emulation process, expected outputs, and potential limitations. Adding verbose explanations would significantly improve usability and troubleshooting capabilities for developers working across different architectures.

@DrXiao
Copy link
Collaborator Author

DrXiao commented May 18, 2025

For the current build system, consider the following table, which lists the valid host-target combinations for building shecc:

host architecture target architecture for shecc Remark
x86-64 armv7 use qemu-arm to emulate
x86-64 riscv use qemu-riscv to emulate
armv7 armv7 run directly on the host computer

Adding verbose explanations would significantly improve usability and troubleshooting capabilities for developers working across different architectures.

If I understood correctly, for example:

  • Use a computer with an armv7 architecture to build shecc targeting riscv.
  • Use a computer with a powerpc architecture to build shecc targeting armv7 or riscv.

The above builds should be considered invalid because these host-target combinations are not listed in the table.

That is, in the first case, we do not allow developers to build shecc targeting riscv on an ARM machine. In the second case, we also do not allow developers build shecc targeting arm or riscv on a non-x86-64 machine.

Therefore, in such cases, the build system should stop and output a warning message to inform the developer.

Is my understanding correct?

@jserv
Copy link
Collaborator

jserv commented May 18, 2025

Is my understanding correct?

Yes, all architectures/platforms are capable of running stage0 once an ordinary C compiler is found. There is explicitly a notable prerequisite to enter stage1 and eventually the following stage2.

Although the previous Makefile used the TARGET_EXEC variable to
store the name of an architecture-specific qemu executable, such as
qemu-arm or qemu-riscv32, for compiling the stage 2 compiler of shecc,
the build system didn't assign the variable correctly, causing the
bootstrapping process to execute the stage 1 compiler directly.

The above issue can be observed by running "make VERBOSE=1" to view
all build steps:

$ make VERBOSE=1
...
...
out/shecc-stage1.elf -o out/shecc-stage2.elf src/main.c
...

Therefore, these changes modify Makefile and .mk files to fix this
issue. After specifying the target architecture, the corresponding
.mk file now assigns the TARGET_EXEC variable directly, ensuring
the build system invokes qemu as expected.
@DrXiao
Copy link
Collaborator Author

DrXiao commented May 18, 2025

I have adjusted the validation approach. The build system will check the required packages and print the warning messages when make config. However, if qemu is required but missing, the build will proceed up to stage 0, print warning messages, and stop at the beginning of stage 1.

I tested the behavior using my beaglebone black (running Debian OS) by running make ARCH=riscv, and the build system behaved as expected:

debian@BeagleBone:~/shecc$ make ARCH=riscv
env printf "ARCH=riscv" > .session.mk
Target machine code switch to riscv
Warning: missing packages: dot jq qemu-riscv32
Warning: Please check package installation
  CC+LD out/inliner
  GEN   out/libc.inc
  CC    out/src/main.o
  LD    out/shecc
Warning: failed to build the stage 1 and stage 2 compilers due to missing qemu-riscv32
make: *** [Makefile:108: out/shecc-stage1.elf] Error 1

If running make again, the build system simply shows the warning.

debian@BeagleBone:~/shecc$ make
Warning: failed to build the stage 1 and stage 2 compilers due to missing qemu-riscv32
make: *** [Makefile:108: out/shecc-stage1.elf] Error 1

In addition, becasue the Debian system does not have graphviz and jq, the relevant warning messages (Warning: missing packages: dot jq qemu-riscv32) are also printed during make config.

Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Both mk/arm.mk and mk/riscv.mk have common rules, and it is better to use define to unify them.

To prevent users from building shecc without certain required packages,
these changes modify the build system to check for prerequisites
when building.

The build system now lists all necessary tools, verify the existence
of their executables, and prints warning messages if any required tool
is missing.

If qemu is required but missing, the build process will proceed only
up to stage 0, print warning messages and stop at the beginning of
stage 1.

Additionally, these changes modify main.yml to install all necessary
tools for the host-arm build, so the build won't fail due to missing
packages.

Close sysprog21#44
@@ -39,7 +39,7 @@ jobs:
githubToken: ${{ github.token }}
install: |
apt-get update -qq -y
apt-get install -yqq build-essential
apt-get install -yqq build-essential graphviz jq
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change may become unnecessary, and I will fix them.

Comment on lines +28 to +39
ifneq ($(HOST_ARCH),$(ARCH_NAME))
# Add qemu to the list if cross compilation
PREREQ_LIST += $(ARCH_QEMU)
ifeq ($(filter $(ARCH_QEMU),$(notdir $(shell which $(ARCH_QEMU)))),)
STAGE1_WARN_MSG := "Warning: failed to build the stage 1 and $\
stage 2 compilers due to missing $(ARCH_QEMU)\n"
STAGE1_CHECK_CMD := $(VECHO) $(STAGE1_WARN_MSG) && exit 1
endif

# Generate the path of an architecture-specific qemu
TARGET_EXEC = $(shell which $(ARCH_QEMU))
endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use 4 spaces for indention.

PREREQ_EXEC := $(shell which $(PREREQ_LIST))
PREREQ_MISSING := $(filter-out $(notdir $(PREREQ_EXEC)),$(PREREQ_LIST))

ifdef PREREQ_MISSING
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto. Be aware of the consistent indention.

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.

Validate the prerequisites when "make config" is executed
2 participants