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

pkg: blacklist selected pkgs for LLVM/clang #9734

Merged
merged 5 commits into from
Sep 21, 2018

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Aug 7, 2018

Contribution description

Some pkgs have problems when compiled with LLVM/clang. This fixes that by blacklisting LLVM for them.

This is still WIP:

  • for some boards it works. Since most of the issues we encounter are assembly related, this might be fixable by filtering by CPU(-family)/instruction set and make the addition to the blacklist dependent on that similar as we did with qDSA already.

Issues/PRs references

Depends on #9730 (merged).

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: pkg Area: External package ports Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … labels Aug 7, 2018
@miri64 miri64 requested a review from cladmi August 7, 2018 16:06
@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 7, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from 378b1db to d76827c Compare August 7, 2018 16:45
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 7, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from 1485801 to 0d1cb3f Compare August 7, 2018 17:21

# There are problem with unused `-mcpu...` arguments in clang and with
# ranlib + LLVM/clang in this package
TOOLCHAINS_BLACKLIST += llvm
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to fix the conversion error first, before tackling this one, but I had ranlib problems in openthread regardless, so it might also be just cortex-m%.

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 8, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from d82b9d2 to 3379a8d Compare August 8, 2018 09:23

ifneq (,$(filter cortex-m%,$(CPU_ARCH)))
# There is a linking issue to `_setjmp()` and `stderr` when compiling
# for Cortex-M with LLVM/clang
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see the error with samr21-xpro:

make -C examples/javascript BOARD=samr21-xpro distclean all TOOLCHAIN=llvm

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhh... now that I went through all that trouble fixing LLVM in jerryscript, I kind of want to know what's causing this, especially since their test with RIOT doesn't seem to have that problem :-/.

Copy link
Contributor

Choose a reason for hiding this comment

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

Posted in the wrong PR before:

With #9821 it looks like I can correctly compile examples/javascript with llvm both on ubuntu 16.04 and in the docker image with BOARD=samr21-xpro.

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 8, 2018
@@ -8,3 +8,9 @@ ifneq (,$(filter openthread_contrib,$(USEMODULE)))
DIRS += $(OPENTHREAD_DIR)/contrib
DIRS += $(OPENTHREAD_DIR)/contrib/netdev
endif

ifneq (,$(filter cortex-m0%,$(CPU_ARCH)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should also be disabled for cortex-m3 it is failing when running buildtest on the iotlab/fox boards.

@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch 2 times, most recently from 9273d16 to 6d90988 Compare August 12, 2018 10:35
@miri64
Copy link
Member Author

miri64 commented Aug 12, 2018

Rebased to current master and dependencies

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 12, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 13, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 14, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from 6d90988 to bcd29d9 Compare August 16, 2018 14:47
@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 16, 2018
@cladmi cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 16, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from bcd29d9 to 031c730 Compare August 16, 2018 15:17
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 16, 2018
@miri64
Copy link
Member Author

miri64 commented Aug 16, 2018

Rebased to current master as #9730 was merged.

Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

Except libcose, I get the same errors.

I re-trigger murdock to see if I can just remove qDSA and make it build (in #9398).

# There is a problem with the LLVM assembler, the M0(+) instruction set, and
# the assembly part of this package
TOOLCHAINS_BLACKLIST += llvm
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not have any error when building

make -C tests/unittests tests-libcose BOARD=samr21-xpro distclean all TOOLCHAIN=llvm

With arm-none-eabi-gcc 6.3.1 and clag 3.8.0 on ubuntu xenial.

But it fails with qDSA though. So could have been a problem of parallel output in murdock maybe ?
It says llvm-as unrecognized option -mcpu=cortex-m0plus. Same on arch linux.

Copy link
Contributor

Choose a reason for hiding this comment

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

Murdock is happy when I removed qDSA https://ci.riot-os.org/RIOT-OS/RIOT/9398/44552886b429f1ce7c704249beb43964fed7a297/output/compile/tests/unittests/samr21-xpro:llvm.txt so it should be pkg/qDSA disabled instead.

make -C tests/unittests tests-qDSA BOARD=samr21-xpro distclean all TOOLCHAIN=llvm

I disabled the test in DISABLE_TEST_FOR_ARM_CORTEX_M for unittests.

I will re-enable the qDSA test to verify.

Copy link
Contributor

Choose a reason for hiding this comment

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

RIOT/tests/unittests/bin/pkg/samr21-xpro/qDSA/arm/asm/bigint_mul.S:17:3: error: invalid instruction, any one of the following would fix this:
  add r0,#8


ifneq (,$(filter cortex-m%,$(CPU_ARCH)))
# There is a linking issue to `_setjmp()` and `stderr` when compiling
# for Cortex-M with LLVM/clang
Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see the error with samr21-xpro:

make -C examples/javascript BOARD=samr21-xpro distclean all TOOLCHAIN=llvm

@@ -31,3 +31,6 @@ DIRS += \
$(NORDIC_SRCS)/components/softdevice/common/softdevice_handler \
$(NORDIC_SRCS)/components/ble/common \
$(NORDIC_SRCS)/components/iot/ble_ipsp

# LLVM ARM assembler has massive problems digesting this
TOOLCHAINS_BLACKLIST += llvm
Copy link
Contributor

Choose a reason for hiding this comment

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

OK it fails for examples/gnrc_networking on nrf52dk.

@@ -1 +1,7 @@
INCLUDES += -I$(PKGDIRBASE)/micro-ecc

ifneq (,$(filter cortex-m0%,$(CPU_ARCH)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok it fails for samr21-xpro in tests/pkg_micro-ecc.

ifneq (,$(filter cortex-m0% cortex-m3%,$(CPU_ARCH)))
# There are problem with unused `-mcpu...` arguments in clang and with
# ranlib + LLVM/clang in this package with Cortex-M0 and M3
TOOLCHAINS_BLACKLIST += llvm
Copy link
Contributor

Choose a reason for hiding this comment

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

I get the same for pkg/openthread with samr21-xpro and iotlab-m3

@miri64
Copy link
Member Author

miri64 commented Aug 16, 2018

From what I've gathered from your review: I removed the blacklist for libcose and added one for qDSA instead.

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 16, 2018
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 16, 2018
@miri64
Copy link
Member Author

miri64 commented Aug 16, 2018

With the current version of this PR, #9398 (1394027) still fails due to libcose, so I revert 3a0c1fa, though I can confirm that qDSA is also failing when compiled locally.

@cladmi
Copy link
Contributor

cladmi commented Aug 17, 2018

@miri64 if qDSA is blacklisting the toolchain, why would even the unittest try to build ?

@miri64
Copy link
Member Author

miri64 commented Aug 17, 2018

@miri64 if qDSA is blacklisting the toolchain, why would even the unittest try to build ?

I only blacklisted Cortex-M0+... but true that's the architecture samr21-xpro is based upon 😕

@miri64
Copy link
Member Author

miri64 commented Aug 17, 2018

arghs... used ifneq instead of ifeq. Fixed and retriggered tests on murdock.

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 22, 2018
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from b42f494 to b32d273 Compare September 9, 2018 14:12
miri64 and others added 4 commits September 9, 2018 16:15
LLVM/clang can't handle the inline assembler instructions in this
package
There is a problem with ranlib and LLVM/clang in this package
@miri64 miri64 force-pushed the pkg/fix/blacklist-llvm branch from b32d273 to ed5dde0 Compare September 9, 2018 14:15
@miri64
Copy link
Member Author

miri64 commented Sep 9, 2018

Rebased and removed jerryscript blacklisting to check if #9821 works now.

miri64 added a commit to miri64/RIOT that referenced this pull request Sep 9, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Sep 11, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Sep 11, 2018
@cladmi cladmi added this to the Release 2018.10 milestone Sep 19, 2018
@miri64
Copy link
Member Author

miri64 commented Sep 21, 2018

@cladmi readded jerryscript to black list, as discussed in #9809 (comment)

@cladmi
Copy link
Contributor

cladmi commented Sep 21, 2018

Looks good to me, and I already tested the blackilsted packages.
Only waiting for #9809 testing it before ACK.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 21, 2018
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK, tested they are needed and checked that they are enough to make murdock happy with llvm #9809

@cladmi cladmi merged commit fc32f81 into RIOT-OS:master Sep 21, 2018
@miri64 miri64 deleted the pkg/fix/blacklist-llvm branch September 21, 2018 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants