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

wasm: restore support for some targets #8401

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

ThomasDevoogdt
Copy link
Contributor

@ThomasDevoogdt ThomasDevoogdt commented Jan 21, 2024

Somehow, support for ARC, MIPS, and XTENSA got dropped by bumping to v1.3.0, so restore it now. Remark that those targets are mentioned in the section above.

See commit fa6a248.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • [N/A] Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/wasm-target branch 2 times, most recently from 2b3d2cb to d1beedb Compare January 21, 2024 17:36
@ThomasDevoogdt ThomasDevoogdt changed the title src/wasm/CMakeLists.txt: restore target support for ARC, MIPS, and XT… wasm: restore target support for ARC, MIPS, and XT… Jan 21, 2024
Somehow, support for ARC, MIPS, and XTENSA got dropped by bumping to v1.3.0,
so restore it now. Remark that those targets are mentioned in the section above.

See commit fa6a248.

Signed-off-by: Thomas Devoogdt <[email protected]>
@ThomasDevoogdt ThomasDevoogdt changed the title wasm: restore target support for ARC, MIPS, and XT… wasm: restore support for some targets Jan 21, 2024
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

These added lines should be failed due to this line:

message(SEND_ERROR "Unsupported build target platform!")

Shouldn't we pay attention to the previous lines to detect automatically platforms?

@ThomasDevoogdt
Copy link
Contributor Author

These added lines should be failed due to this line:

Wrong, that block is guarded with "if (NOT DEFINED WAMR_BUILD_TARGET)", but I do define it.
See: https://github.com/buildroot/buildroot/blob/master/package/fluent-bit/fluent-bit.mk#L25-L39.

@cosmo0920
Copy link
Contributor

cosmo0920 commented Jan 22, 2024

These added lines should be failed due to this line:

Wrong, that block is guarded with "if (NOT DEFINED WAMR_BUILD_TARGET)", but I do define it. See: https://github.com/buildroot/buildroot/blob/master/package/fluent-bit/fluent-bit.mk#L25-L39.

I mean, this should be completed inside on the CMakeLists.txt not on the additional autoconf mechanism.

@ThomasDevoogdt
Copy link
Contributor Author

I mean, this should be completed inside on the CMakeLists.txt not on the additional autoconf mechanism.

The problem is that the CMakeLists.txt was not really able to detect the relevant platforms right. Partially since buildroot sometimes uses other names and so on. So for that reason, this translation is done:

config BR2_PACKAGE_FLUENT_BIT_WASM_ARCH
	string
	default "AARCH64"    if BR2_aarch64 || BR2_aarch64be
	default "ARC"        if BR2_arcle || BR2_arceb
	default "ARM"        if BR2_arm || BR2_armeb
	default "MIPS"       if BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el
	default "RISCV32"    if BR2_riscv && BR2_RISCV_32
	default "RISCV64"    if BR2_riscv && BR2_RISCV_64
	default "X86_32"     if BR2_i386
	default "X86_64"     if BR2_x86_64
	default "XTENSA"     if BR2_xtensa
...
-DWAMR_BUILD_TARGET=$(FLUENT_BIT_WAMR_ARCH)

I now wanted to bump fluent-bit from v2.1.7 to v2.2.2, and saw that some targets were broken.
The main reason seems to be that some of them were dropped from the list.

So there are indeed two things here.
a) the auto detection which is not yet solved by this PR to detect ARC, MIPS, and XTENSA, should be added here:

# Set WAMR_BUILD_TARGET, currently values supported:
# "X86_64", "AMD_64", "X86_32", "AARCH64[sub]", "ARM[sub]", "THUMB[sub]",
# "MIPS", "XTENSA", "RISCV64[sub]", "RISCV32[sub]"
if (NOT DEFINED WAMR_BUILD_TARGET)
if (CMAKE_SYSTEM_PROCESSOR MATCHES "^(arm64|aarch64)")
set (WAMR_BUILD_TARGET "AARCH64")
# For raspbian/buster: armv6l-unknown-linux-gnueabihf
elseif (CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv6.*|armv7.*)")
set (WAMR_BUILD_TARGET "ARM")
elseif (CMAKE_SYSTEM_PROCESSOR STREQUAL "riscv64")
set (WAMR_BUILD_TARGET "RISCV64")
elseif (CMAKE_SIZEOF_VOID_P EQUAL 8)
# Build as X86_64 by default in 64-bit platform
set (WAMR_BUILD_TARGET "X86_64")
elseif (CMAKE_SIZEOF_VOID_P EQUAL 4)
# Build as X86_32 by default in 32-bit platform
set (WAMR_BUILD_TARGET "X86_32")
else ()
message(SEND_ERROR "Unsupported build target platform!")
endif ()
endif ()

b) the definitions for the build target (this PR), which is part of this block:
# Add definitions for the build target
if (WAMR_BUILD_TARGET STREQUAL "X86_64")
add_definitions(-DBUILD_TARGET_X86_64)
elseif (WAMR_BUILD_TARGET STREQUAL "AMD_64")
add_definitions(-DBUILD_TARGET_AMD_64)
elseif (WAMR_BUILD_TARGET STREQUAL "X86_32")
add_definitions(-DBUILD_TARGET_X86_32)
elseif (WAMR_BUILD_TARGET MATCHES "AARCH64.*")
add_definitions(-DBUILD_TARGET_AARCH64)
add_definitions(-DBUILD_TARGET="${WAMR_BUILD_TARGET}")
elseif (WAMR_BUILD_TARGET MATCHES "ARM.*")
add_definitions(-DBUILD_TARGET_ARM)
add_definitions(-DBUILD_TARGET="${WAMR_BUILD_TARGET}")
elseif (WAMR_BUILD_TARGET STREQUAL "RISCV64" OR WAMR_BUILD_TARGET STREQUAL "RISCV64_LP64D")
add_definitions(-DBUILD_TARGET_RISCV64_LP64D)
elseif (WAMR_BUILD_TARGET STREQUAL "RISCV64_LP64")
add_definitions(-DBUILD_TARGET_RISCV64_LP64)
elseif (WAMR_BUILD_TARGET STREQUAL "RISCV32" OR WAMR_BUILD_TARGET STREQUAL "RISCV32_ILP32D")
add_definitions(-DBUILD_TARGET_RISCV32_ILP32D)
elseif (WAMR_BUILD_TARGET STREQUAL "RISCV32_ILP32")
add_definitions(-DBUILD_TARGET_RISCV32_ILP32)
else ()
message (FATAL_ERROR "-- Build target isn't set")
endif ()

@cosmo0920
Copy link
Contributor

cosmo0920 commented Jan 22, 2024

Ah, got it. Thanks for the explanation. Then, it seems great to detect for the latter detectors with user-defined architecture variable. The auto detectors are only good enough to handle well-known architectures, I believe.

@edsiper edsiper merged commit 40bb0fb into fluent:master Jan 22, 2024
35 of 36 checks passed
@edsiper
Copy link
Member

edsiper commented Jan 22, 2024

thank you. I will backport this to 2.2

edsiper pushed a commit that referenced this pull request Jan 22, 2024
Somehow, support for ARC, MIPS, and XTENSA got dropped by bumping to v1.3.0,
so restore it now. Remark that those targets are mentioned in the section above.

See commit fa6a248.

Signed-off-by: Thomas Devoogdt <[email protected]>
edsiper pushed a commit that referenced this pull request Jan 22, 2024
Somehow, support for ARC, MIPS, and XTENSA got dropped by bumping to v1.3.0,
so restore it now. Remark that those targets are mentioned in the section above.

See commit fa6a248.

Signed-off-by: Thomas Devoogdt <[email protected]>
shaerpour pushed a commit to shaerpour/fluent-bit that referenced this pull request Jan 25, 2024
Somehow, support for ARC, MIPS, and XTENSA got dropped by bumping to v1.3.0,
so restore it now. Remark that those targets are mentioned in the section above.

See commit fa6a248.

Signed-off-by: Thomas Devoogdt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants