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

Update Zephyr support to v3.5.0 and make instructions generic to boards #2805

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

hasheddan
Copy link
Contributor

Includes a number of updates to the Zephyr example, including removing a fair amount of stale configuration. This is part of moving towards supporting WAMR as a Zephyr module as described in #2782. Some functionality is removed as part of this diff (e.g. AOT XTENSA support), but it had become stale while not being actively maintained. It is anticipated that it (and AOT support for other platforms) will be added back, along with CI support for ensuring they do not become stale again.

See individual commits for more specific information about changes.

cc @wenyongh

This Dockerfile is using a version of Zephyr that is no longer
supported, and the various parts of WAMR may no longer work with it.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Updates the Zephyr Dockerfile to v3.5.0 and the latest SDK version,
which does not require manually installing the Espressif toolchain.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>

```shell
sudo apt-get install qemu
west build . -b <board-identifier> --pristine -- -DWAMR_BUILD_TARGET=<wamr-arch>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to add a step to copy the boards/<board>.conf to prj.conf so that a newbie like me won't forget? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah great catch! I meant to just had an empty prj.conf, which should alleviate this issue :) I may also move some of the config that is not specific to a single board into the prj.conf 👍🏻

@TianlongLiang
Copy link
Contributor

TianlongLiang commented Nov 21, 2023

Thanks for this amazing PR, it definitely makes the Zephyr support much better and cleaner. One small problem though, I tested the esp32_devkitm build:

west build -b esp32c3_devkitm . --pristine -- -DWAMR_BUILD_TARGET=RISCV32_ILP32

And find a linking issue:

/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c: Assembler messages:
/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c:180: Error: unrecognized opcode `fence.i'
/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c:180: Error: unrecognized opcode `fence.i'
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /

Maybe we can modify the relocation opcode for RISCV or add some flags to support this opcode in CMake, what do you think?

@no1wudi
Copy link
Collaborator

no1wudi commented Nov 21, 2023

@TianlongLiang Maybe we should not use fence.i directly since it has been removed from rv32i standard, I'll submit a fix.

@TianlongLiang
Copy link
Contributor

@TianlongLiang Maybe we should not use fence.i directly since it has been removed from rv32i standard, I'll submit a fix.

Good advice, let’s choose this path

@no1wudi
Copy link
Collaborator

no1wudi commented Nov 21, 2023

@TianlongLiang Could you try this patch?

#2807

@TianlongLiang
Copy link
Contributor

@TianlongLiang Could you try this patch?

#2807

Hi, @no1wudi I tried the patch, and still have this error, it seems that the target attribute wasn't set successfully, I am not sure whether this is because of the compiler:

/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c:188:1: warning: target attribute is not supported on this machine [-Wattributes]
  188 | {
      | ^
/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c: Assembler messages:
/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c:192: Error: unrecognized opcode `fence.i'
/root/wasm-micro-runtime/core/iwasm/aot/arch/aot_reloc_riscv.c:192: Error: unrecognized opcode `fence.i'

The Zephyr is using the gcc in its sdk:

-- Found Git: /usr/bin/git (found version "2.34.1") 
-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /opt/zephyr-sdk-0.16.3/riscv64-zephyr-elf/bin/riscv64-zephyr-elf-gcc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/zephyr-sdk-0.16.3/riscv64-zephyr-elf/bin/riscv64-zephyr-elf-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /opt/zephyr-sdk-0.16.3/riscv64-zephyr-elf/bin/riscv64-zephyr-elf-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building ESP-IDF components for target esp32c3

@no1wudi
Copy link
Collaborator

no1wudi commented Nov 21, 2023

@TianlongLiang Could you try this patch?
#2807

Hi, @no1wudi I tried the patch, and still have this error, it seems that the target attribute wasn't set successfully, I am not sure whether this is because of the compiler:

@TianlongLiang Seems the zephyr's toolchain is not support this attribute, please try the update again.

@TianlongLiang
Copy link
Contributor

@TianlongLiang Could you try this patch?
#2807

Hi, @no1wudi I tried the patch, and still have this error, it seems that the target attribute wasn't set successfully, I am not sure whether this is because of the compiler:

@TianlongLiang Seems the zephyr's toolchain is not support this attribute, please try the update again.

@no1wudi Now it can be built successfully 😄

@hasheddan
Copy link
Contributor Author

@TianlongLiang @no1wudi thanks for y'all's reviews! I don't think we need to necessarily remove the fence.i instruction. It just happens that esp32c3_devkitm does not support the zifencei instruction, but some other targets, such as QEMU, do support zifencei. This PR mentions that AOT must be disabled on some targets, and provides the necessary config to do so:

It and other
options can be disabled by modifying the CMakeLists.txt
file, or by passing additional arguments at build (e.g. -DWAMR_BUILD_AOT=0).

Using west build -b esp32c3_devkitm . --pristine -- -DWAMR_BUILD_TARGET=RISCV32_ILP32 -DWAMR_BUILD_AOT=0 should work correctly 🙂

@hasheddan
Copy link
Contributor Author

@TianlongLiang @no1wudi I have updated to include the prj.conf here as well. I don't think we necessarily need to block on #2807 given that supplying -DWAMR_BUILD_AOT=0 is described in the instructions. I'm not entirely confident that just removing the fence.i instruction means that AOT will work correctly on these targets, even if it does compile. I think it makes sense to validate that as a separate effort.

@TianlongLiang
Copy link
Contributor

@TianlongLiang @no1wudi I have updated to include the prj.conf here as well. I don't think we necessarily need to block on #2807 given that supplying -DWAMR_BUILD_AOT=0 is described in the instructions. I'm not entirely confident that just removing the fence.i instruction means that AOT will work correctly on these targets, even if it does compile. I think it makes sense to validate that as a separate effort.

You are right about we need to test the actual run on the target to validate that it actually works. Unfortunately, I don't have a hardware test environment to test it. Maybe we can mention the current AOT situation for esp32c3_devkitm in README or somewhere for future user reference. They can try the default setting with AOT enabled first, and if it does not work on AOT, they can use other running modes. What's your suggestion?

@@ -1,4 +1,4 @@
# Copyright (C) 2019 Intel Corporation. All rights reserved.
# Copyright (C) 2023 Golioth, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Had better not change this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is showing as a changed line, but this is actually a net new file that is replacing the same config being used across all the board config files in boards/. I am happy to copy over the Intel copyright if preferred though -- let me know 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not so sure, but had better keep unchanged as there is no change in the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has the same config of some of the board files previously did, but is new and gets combined with the remaining board files when a target is matched. I have used the Intel copyright header for now though 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you not delete this file? It is important for us to test whether WAMR runs OK on some qemus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenyongh the absence of this file shouldn't prohibit testing against QEMU targets. They can be built and run along with any other target -- is adding some examples for using QEMU targets in the README.md sufficient here? Also, I don't believe that there are currently any CI tests for Zephyr -- is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated README.md with information about how to use emulated targets 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is not to remove this file, it is really convenient and import for us to test all the targets we want to support, please modify it if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have dropped the commit that removed it and updated it to use latest Zephyr updates (including dropping external toolchain references, fixing missing config, and removing no longer supported targets).

@hasheddan
Copy link
Contributor Author

Maybe we can mention the current AOT situation for esp32c3_devkitm in README or somewhere for future user reference. They can try the default setting with AOT enabled first, and if it does not work on AOT, they can use other running modes. What's your suggestion?

@TianlongLiang added esp32c3_devkitm as an example build target with a note about it not currently supporting AOT 👍🏻

Updates board names that have changed in the most recent Zephyr release
(v3.5.0).

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Removes esp32 conditionals in the Zephyr sample as esp32 is no longer a
board target in latest Zephyr builds, and the custom linker script that
they depended upon has been removed as stale.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Updates the Zephyr README.md to be generic towards all boards rather
than depending on a build script that can easily become stale. We may
opt to have a set of boards that we guarantee builds on in the future,
such as those for which board configuration files are currently
provided. At that time, we should ensure that those boards are tested in
CI.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Rather than copying the board files to prj.conf, all common
configuration is added to the prj.conf and board configuration files are
removed that only contained the common config variables.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Adds some example target builds to README.md and instructions on using
emulated targets.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
The espressif toolchain is now available via the Zephyr SDK and does not
need to be separately installed or referenced.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Removes the no longer supported esp32 target from build script.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Disables AOT support for esp32c3_devkitm as it does not currently
compile successfully.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@wenyongh thanks for your feedback! I believe I have addressed all comments, but let me know if there is anything missing :)

@@ -17,11 +16,10 @@ QEMU_ARC_TARGET="qemu_arc"
usage ()
{
echo "USAGE:"
echo "$0 $X86_TARGET|$STM32_TARGET|$ESP32_TARGET|$ESP32C3_TARGET|$PARTICLE_ARGON_TARGET|$QEMU_CORTEX_A53|$QEMU_XTENSA_TARGET|$QEMU_RISCV64_TARGET|$QEMU_RISCV32_TARGET|$QEMU_ARC_TARGET"
echo "$0 $X86_TARGET|$STM32_TARGET|$ESP32C3_TARGET|$PARTICLE_ARGON_TARGET|$QEMU_CORTEX_A53|$QEMU_XTENSA_TARGET|$QEMU_RISCV64_TARGET|$QEMU_RISCV32_TARGET|$QEMU_ARC_TARGET"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why the esp32 target is removed, is it because that esp32c3 is more popular and no need to support esp32 again, or here esp32c3 target is also suitable for esp32 target?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is a typo, esp32 is xtensa based, esp32c3 is risc-v based, they are different.

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, had better not remove esp32, including changes in src/main.c, boards/esp32.conf and esp32_custom_linker.ld. We had better support both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenyongh @no1wudi there are many different esp32 variants and just esp32 is not a valid target in Zephyr anymore, so build will fail. I would like to work on ensuring that all esp32 variants build correctly, but that will be a larger effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hasheddan got it, thanks, so we had better merge this PR first. Could you help enable the esp32 support after "Make Zephyr port work as a Zephyr module" as mentioned in #2782?

@no1wudi how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you help enable the esp32 support after "Make Zephyr port work as a Zephyr module" as mentioned in #2782?

@wenyongh absolutely!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wenyongh I'm fine with it

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, merged.

@wenyongh wenyongh merged commit 2175910 into bytecodealliance:main Nov 23, 2023
384 checks passed
wenyongh pushed a commit that referenced this pull request Nov 23, 2023
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…ds (bytecodealliance#2805)

Includes a number of updates to the Zephyr example, including removing a fair
amount of stale configuration. This is part of moving towards supporting WAMR
as a Zephyr module as described in bytecodealliance#2782. Some functionality is removed as
part of this diff (e.g. AOT XTENSA support), but it had become stale while not
being actively maintained. It is anticipated that it (and AOT support for other
platforms) will be added back, along with CI support for ensuring they do not
become stale again.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
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.

None yet

4 participants