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

cpu/Makefile.include.cortexm_common: don't use cortex-m0plus for clang if unsupported #3870

Merged
merged 1 commit into from Oct 8, 2015

Conversation

cgundogan
Copy link
Member

My clang version (3.6.2) does not support the cortex-m0plus mcpu type. I have no idea in which version they ~~will/~~have include/ed this, but building any application for the samr21-xpro with llvm/clang breaks for me:

~/r/R/e/gnrc_networking >>> TOOLCHAIN=clang BOARD=samr21-xpro make -B clean all
Building application "gnrc_networking" for "samr21-xpro" with MCU "samd21".

error: unknown target CPU 'cortex-m0plus'

NOTE: I am currently building llvm/clang 3.8 from source and it appears that the m0plus is supported there. So I will just PR this and look what your opinion is on this matter. Close if necessary.

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system labels Sep 16, 2015
@jnohlgard
Copy link
Member

A suggestion: Instead of checking the reported version number, could we check if the -mcpu=cortexm0plus option is supported by checking the exit code from a test call to clang?

@jnohlgard
Copy link
Member

I did some messing around on the command line, the command

clang -target arm-none-eabi -mcpu=cortex-m0plus -c -x c /dev/null -o /dev/null 

will return 0 if all options are valid and 1 when there is an invalid -mcpu option (try it with -mcpu=cortex-m8 and check the exit code)

@cgundogan
Copy link
Member Author

Ah okay, I came up with another solution. Which one do you like more?

ifeq (llvm,$(TOOLCHAIN))
ifeq (cortex-m0plus,$(CPU_ARCH))
LLVM_CHECK = $(shell llvm-as < /dev/null | llc -march=$(CPU_MODEL) -mcpu=$(CPU_ARCH))
ifneq (,LLVM_CHECK)
Copy link
Member

Choose a reason for hiding this comment

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

missing $()

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

@cgundogan
Copy link
Member Author

addressed @gebart's comment

# llvm/clang version 3.6.2 still does not support the cortex-m0plus mcpu type
ifeq (llvm,$(TOOLCHAIN))
ifeq (cortex-m0plus,$(CPU_ARCH))
LLVM_UNSUP = $(shell clang -target arm-none-eabi -mcpu=$(CPU_ARCH) -c -x c /dev/null -o /dev/null \
Copy link
Member

Choose a reason for hiding this comment

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

also redirect stdout and stderr from clang (>/dev/null 2>&1)

  • addressed

@cgundogan cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 17, 2015
@cgundogan
Copy link
Member Author

addressed

@cgundogan cgundogan changed the title cpu/Makefile.include.cortexm_common: don't use cortex-m0plus for clang cpu/Makefile.include.cortexm_common: don't use cortex-m0plus for clang if unsupported Sep 17, 2015
@jnohlgard
Copy link
Member

​ACK, please squash.
Can this go in the release?

@cgundogan cgundogan removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 17, 2015
@cgundogan
Copy link
Member Author

squashed.

Can this go in the release?

Frankly speaking, I don't know.
How critical is this for others? Do you think many users will build with llvm/clang? Archlinux' version of llvm/clang in the official repos is 3.6.2. I just looked up that ubuntu's defaul is also 3.6.2 @OlegHahm What's your opinion?

@OlegHahm
Copy link
Member

Usually, I would say don't import something "new" into master this late in the release process, but on the other hand this shouldn't break anything for our "default toolchain", so I would be okay with merging.

ifeq (cortex-m0plus,$(CPU_ARCH))
LLVM_UNSUP = $(shell clang -target arm-none-eabi -mcpu=$(CPU_ARCH) -c -x c /dev/null -o /dev/null \
> /dev/null 2>&1 || echo 1 )
ifneq (1,$(LLVM_UNSUP))
Copy link
Member

Choose a reason for hiding this comment

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

should be ifeq, I think..

@cgundogan cgundogan added this to the Release NEXT MAJOR milestone Sep 22, 2015
@cgundogan
Copy link
Member Author

addressed @gebart's comment (amended)

@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 5, 2015
@cgundogan
Copy link
Member Author

@gebart does your ACK still hold? (long time since last ACK)

@jnohlgard
Copy link
Member

ACK holds

@cgundogan
Copy link
Member Author

travis is happy > GO

cgundogan added a commit that referenced this pull request Oct 8, 2015
cpu/Makefile.include.cortexm_common: don't use cortex-m0plus for clang if unsupported
@cgundogan cgundogan merged commit 05fe4a3 into RIOT-OS:master Oct 8, 2015
@cgundogan cgundogan deleted the pr/cpu/cortexm0plus branch October 8, 2015 14:50
@sgso
Copy link
Contributor

sgso commented Oct 8, 2015

@cgundogan Does clang know cortex-m0.small-multiply or has an equivalent?

@cgundogan
Copy link
Member Author

@sgso I checked briefly in their doxygen and searched in the github mirror of clang but couldn't find anything equivalent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants