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

cortexm: Add support for building with LLVM/Clang #3119

Merged
merged 1 commit into from Jun 4, 2015

Conversation

jnohlgard
Copy link
Member

as an alternative to the default GCC toolchain.

export BUILD_WITH_CLANG=1
export CC=clang

export TOOLCHAIN=llvm

or

export TOOLCHAIN=clang

to build with Clang instead of GCC

@jnohlgard jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 30, 2015

# define build specific options
export CFLAGS_CPU = -mcpu=$(MCPU) -mlittle-endian -mthumb -mno-thumb-interwork $(CFLAGS_FPU)
export CFLAGS_CPU = -mcpu=$(MCPU) -mlittle-endian -mthumb -fshort-enums $(CFLAGS_FPU)
Copy link
Member Author

Choose a reason for hiding this comment

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

as for -fshort-enums:
newlib nano on my machine seems to be built with -fshort-enums which causes warnings during linking with RIOT built using Clang. It seems like gcc defaults to using short enums and clang defaults to using word size enums because RIOT files built with GCC do not cause any warnings.

@jnohlgard
Copy link
Member Author

is BUILD_WITH_CLANG a suitable environment variable name or is it too long?

@haukepetersen
Copy link
Contributor

I would opt for changing the switches name to something more general, maybe to TOOLCHAIN. Then you coult build with

TOOLCHAIN=gnu make all

or

TOOLCHAIN=llvm make all

or anything else that might come up.

So in the Makefile you could just write:

#default to GNU
TOOLCHAIN ?= gnu
...

include $(RIOTBOARD)/Makefile.include.$(TOOLCHAIN)

how does that sound?

@jnohlgard
Copy link
Member Author

Good idea, I will adapt this pr when I am back at the computer.
On May 30, 2015 3:32 PM, "Hauke Petersen" notifications@github.com wrote:

I would opt for changing the switches name to something more general,
maybe to TOOLCHAIN. Then you coult build with

TOOLCHAIN=gnu make all

or

TOOLCHAIN=llvm

or anything else that might come up.

So in the Makefile you could just write:

#default to GNU
TOOLCHAIN ?= gnu
...

include $(RIOTBOARD)/Makefile.include.$(TOOLCHAIN)

how does that sound?


Reply to this email directly or view it on GitHub
#3119 (comment).

@LudwigKnuepfer
Copy link
Member

Personally, I would prefer to set this based on the value of CC. I.e. when I want to build using llvm, I usually just export CC=clang and be done with it.

@jnohlgard
Copy link
Member Author

@LudwigOrtmann @haukepetersen OK, how about this:
Let TOOLCHAIN be the primary definition, but if TOOLCHAIN is unset or empty, then look at the CC variable and try to deduce the TOOLCHAIN variable from the name there?

@jnohlgard
Copy link
Member Author

(and lastly go with the platform default, i.e. gnu for cortex)

@LudwigKnuepfer
Copy link
Member

Is there any advantage in having TOOLCHAIN in addition to CC? Or vice versa - do you see any problems in using CC only?

@jnohlgard
Copy link
Member Author

@LudwigOrtmann TOOLCHAIN is the more logical name since this affects the entire chain, including compiling C, C++, assembler, linking, objcopy (though llvm doesn't have a proper replacement for that one yet)

Also it is useful inside make to have a variable to check use in ifeq statements for specific compiler options without having to strip the PREFIX etc from it, so there will be a use for a TOOLCHAIN variable inside the build system, might as well let people override it from the environment/command line.

@jnohlgard
Copy link
Member Author

rebased, updated with TOOLCHAIN/CC

@jnohlgard jnohlgard added this to the Release 2015.06 milestone May 31, 2015
@jnohlgard
Copy link
Member Author

Added short description to https://github.com/RIOT-OS/RIOT/wiki/Build-Flags I will edit according to the final state of this PR when merged.

@jnohlgard jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 31, 2015

# define build specific options
export CFLAGS_CPU = -mcpu=$(MCPU) -mlittle-endian -mthumb -mno-thumb-interwork $(CFLAGS_FPU)
export CFLAGS_CPU = -mcpu=$(MCPU) -mlittle-endian -mthumb $(CFLAGS_FPU)
Copy link
Contributor

Choose a reason for hiding this comment

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

if this line is dependent on the toolchain, I am wondering if it would make it easier, to move this line to Makefile.include.gnu and Makefile.include.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.

If we move it, I think it will be easily overlooked that there are two places to modify if we need to change any flags. Also, it makes reading the files more tedious (switching back and forth between two/three files in the editor)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should keep cortex stuff out of Makefile.common.gnu/llvm, as the latter might be reused for non-cortex cpus.

@haukepetersen
Copy link
Contributor

By looking at the Makefile now, I am not sure if I am a big fan of deducting the TOOLCHAIN from CC. It does not make the Makefile more readable while adding in my opinion not really much additional benefit. So I would actually vote for using the TOOLCHAIN variable only for selecting the correct toolchain specific sub-Makfile. The CC option can still be used for selecting the compiler, but only the compiler and that is what this option is for in the first place!

@jnohlgard
Copy link
Member Author

@haukepetersen the way the board makefiles are written the CC variable can't actually be used for selecting the compiler since it is always overridden in the toolchain makefiles (both before and after this PR)

@jnohlgard
Copy link
Member Author

@haukepetersen I do agree though that the CC checking block is making the file a bit more complex without adding much of a feature. All that is needed is export TOOLCHAIN=llvm instead of CC=clang

I also added export TOOLCHAIN=clang as an alias because it would be logical for users to believe that the toolchain would be called clang when the compiler command is clang.

@jnohlgard
Copy link
Member Author

@LudwigOrtmann what is your opinion on this?
Are you OK with removing the big ifeq [CC] block from the Makefile?

@LudwigKnuepfer
Copy link
Member

@gebart yes

@jnohlgard
Copy link
Member Author

rebased, removed the CC checks and squashed the previous commits. OK to squash all?

@@ -1,18 +1,34 @@
# Target triple for the build. Use arm-none-eabi if you are unsure.
export TARGET_TRIPLE ?= arm-none-eabi

# Toolchain prefix, defaults to target triple followed by a dash, you will most likely not need to touch this.
# Use TOOLCHAIN environment variable to select the toolchain to use.
ifeq (,$(TOOLCHAIN))
Copy link
Contributor

Choose a reason for hiding this comment

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

TOOLCHAIN ?= gnu saves some lines here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought: many build systems use TOOLCHAIN for the actual triple prefix (e.g., arm-none-eabi-). But I can't think of a better name. Opinions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't think of a better name either, and I have not seen the TOOLCHAIN=arch-prefix- usage yet. Is it widespread?


ifeq (,$(PREFIX))
# default toolchain prefix, defaults to target triple followed by a dash, you
# will most likely not need to touch this.
export PREFIX ?= $(if $(TARGET_TRIPLE),$(TARGET_TRIPLE)-)
Copy link
Contributor

Choose a reason for hiding this comment

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

The ?= and ifeq above are redundant, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, addressed.

@kaspar030
Copy link
Contributor

OK to squash all?

Yeah, lets merge and improve later.

@jnohlgard
Copy link
Member Author

Addressed some comments.

  • Move newlib include search to sys/newlib/Makefile.include
  • Add override for TOOLCHAIN=clang to force TOOLCHAIN=llvm
  • small comment/whitespace fixes.

@jnohlgard
Copy link
Member Author

I am happy with the current state now. OK to squash again?

@kaspar030
Copy link
Contributor

yes!

as an alternative to the default GCC toolchain.

    export TOOLCHAIN=llvm

to build with Clang instead of GCC
@jnohlgard
Copy link
Member Author

rebased, squashed, ready

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

Travis is green, any ACKs?

@haukepetersen
Copy link
Contributor

ACK and go.

haukepetersen added a commit that referenced this pull request Jun 4, 2015
cortexm: Add support for building with LLVM/Clang
@haukepetersen haukepetersen merged commit d8532ea into RIOT-OS:master Jun 4, 2015
@jnohlgard jnohlgard deleted the pr/cortexm-clang branch June 25, 2015 07:58
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: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants