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

Compile apps for RISC-V with make flag #88

Merged
merged 50 commits into from
Jun 17, 2020
Merged

Compile apps for RISC-V with make flag #88

merged 50 commits into from
Jun 17, 2020

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jun 5, 2020

This adds rv32imac binaries to TABs from libtock-c. These are compiled for specific flash and RAM addresses.

The main change for doing this is automatically generating the linker script when building for each architecture. This copies the default linker script and sets the addresses if needed. Different compilation versions are set in the Configuration.mk makefile.

This also compiles libraries for rv32imac, and updates some apps that use assembly.

TODO

  • fix crt0 header for riscv, need to take the stack size into account
  • make got just a copy
  • fix lst build for cortex-m

README.md Outdated Show resolved Hide resolved
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

I didn't test this yet as I was hoping to skip brew debugging.

From just reading it, it looks like this will force a recompile every time make is run? (Because the binary depends on the *.custum.ld and the rule that makes that file runs unconditionally). I wouldn't expect to recompile if nothing has changed though.

examples/tests/stack_size_test02/main.c Outdated Show resolved Hide resolved
examples/lua-hello/Makefile Outdated Show resolved Hide resolved
examples/ble_passive_scanning/Makefile Outdated Show resolved Hide resolved
AppMakefile.mk Outdated Show resolved Hide resolved
AppMakefile.mk Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Jun 8, 2020

Ok it seems like the main issue is how to install toolchains, and make sure they work on both mac and linux. There was no pushback on #44, so I'm assuming there is no reservations about supporting both, we just need reasonable install steps.

If anyone knows of a trick for installing on Linux, please let me know.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 8, 2020

I don't even know where to post an issue (or comment on one) asking the toolchain maintainers to make this easier. But, I'm kind of thinking we just host our own build for ubuntu.

Copy link
Contributor

@alistair23 alistair23 left a comment

Choose a reason for hiding this comment

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

Why are you checking in some binary files as well?

.travis-install-gcc Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Configuration.mk Outdated Show resolved Hide resolved
Configuration.mk Outdated Show resolved Hide resolved
Configuration.mk Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Jun 8, 2020

We pre-compile binaries (nrf serialization for example) to make it easier for users.

@alistair23
Copy link
Contributor

How do you handle different RISC-V ISA strings in the pre-compiled binaries? For example supporting both OT and the HiFive1?

@bradjc
Copy link
Contributor Author

bradjc commented Jun 8, 2020

It's no different than cortex-m3 vs m4 (at least it shouldn't be).

@alistair23
Copy link
Contributor

Good point.

Just watch out as there are a lot of different combinations that will have to be built and maintained as RISC-V support grows.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
$ sudo apt install gcc-riscv64-unknown-elf
```

Unfortunately, Ubuntu does not currently (June 2020) provide a package for
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably best to specify the version as earlier then 20.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if this isn't fixed in 20.10?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we will have to update it, but a date has the same problem. What if 20.10 works? It's then not clear to people that it should work for them.

Either way though we will need to update the steps.

@bradjc bradjc added the blocked Blocked on promised changes or other PRs label Jun 12, 2020
libtock/crt0.c Outdated Show resolved Hide resolved
userland_generic.ld Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@ppannuto
Copy link
Member

ppannuto commented Jun 14, 2020

I don't this TOCK_TARGETS is working as expected? Since I don't currently have a working riscv toolchain, I tried building the hail app to see if other stuff still worked, but that resulted in:

[-bash] Sun 14 Jun 16:10 [[riscv-for-all] ~/code/helena-project/libtock-c/examples/tests/hail]
$ make
  CC        ../../../libtock/internal/alarm_internal.c
/bin/sh: riscv64-unknown-elf-gcc: command not found
make: *** [../../../libtock/build/rv32imac/alarm_internal.o] Error 127

If riscv isn't in the targets list, then its toolchain shouldn't be being invoked at all.

Verbose output
[-bash] Sun 14 Jun 16:10 [[riscv-for-all] ~/code/helena-project/libtock-c/examples/tests/hail]
$ make V=1
**************************************************
TOCK USERLAND BUILD SYSTEM -- VERBOSE BUILD
**************************************************
Config:
CC=-gcc
LAYOUT=../../../userland_generic.ld
MAKEFLAGS= -r -R
PACKAGE_NAME=hail
TOCK_ARCHS=cortex-m0 cortex-m3 cortex-m4 rv32imac rv32imc
TOCK_TARGETS=cortex-m4
TOCK_USERLAND_BASE_DIR=../../..
TOOLCHAIN=
**************************************************

riscv64-unknown-elf-gcc -std=gnu11 -Wbad-function-cast -Wjump-misses-init -Wmissing-prototypes -Wnested-externs -Wold-style-definition -D__TOCK__ -DSVCALL_AS_NORMAL_FUNCTION -DSOFTDEVICE_s130 -I../../../libnrfserialization/headers -frecord-gcc-switches -gdwarf-2 -Os -fdata-sections -ffunction-sections -fstack-usage -Wstack-usage=2048 -Wall -Wextra -Wl,--warn-common -Wl,--gc-sections -Wl,--emit-relocs -fPIC -include ../../../support/warning_header.h -Wdate-time -Wfloat-equal -Wformat-nonliteral -Wformat-security -Wformat-y2k -Winit-self -Wlogical-op -Wmissing-declarations -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-noreturn -Wmultichar -Wpointer-arith -Wredundant-decls -Wshadow -Wtrampolines -Wunused-macros -Wunused-parameter -Wwrite-strings -I../../../libtock/ -I../../../libtock//include -I../../../libtock//../include -I../../../libtock/internal/ -I../../../libtock/internal//include -I../../../libtock/internal//../include -march=rv32imac -mabi=ilp32 -mcmodel=medlow -Wl,--no-relax -MF"../../../libtock/build/rv32imac/alarm_internal.d" -MG -MM -MP -MT"../../../libtock/build/rv32imac/alarm_internal.d@" -MT"../../../libtock/build/rv32imac/alarm_internal.o" "../../../libtock/internal/alarm_internal.c"
/bin/sh: riscv64-unknown-elf-gcc: command not found
make: *** [../../../libtock/build/rv32imac/alarm_internal.o] Error 127

README.md Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Jun 14, 2020

I don't this TOCK_TARGETS is working as expected? Since I don't currently have a working riscv toolchain, I tried building the hail app to see if other stuff still worked, but that resulted in:

TARGETS works together with the TOCK_ARCHS, you have to remove it there as well.

@ppannuto
Copy link
Member

TARGETS works together with the TOCK_ARCHS, you have to remove it there as well.

I guess I don't understand why these should be different things?

@ppannuto
Copy link
Member

Filed riscvarchive/riscv-gcc#190 for ICE

@ppannuto
Copy link
Member

Got it. I pushed a commit so that TOCK_ARCHS populates automatically from TOCK_TARGETS, that way if there's no riscv target (e.g. lua), only the arm toolchains are used.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 16, 2020

So I think the only issue left is installing a toolchain?

  • Ubuntu: It would be great if someone could test the ubuntu steps.
  • Mac: The steps work generally, but since the brew formula isn't pegged to a version asynchronous updates to the riscv/riscv-gnu-toolchain repo can lead to a buggy compiler. It also takes forever to compile. I asked about maybe fixing that second one: Include the 32 bit tools? riscv-software-src/homebrew-riscv#22. One option is we could create a homebrew-tock repo and provide our own bottles with multilib support that are pegged to a version (or commit). We could also then provide formulas for all of our tools, and the entire getting started install becomes brew tap tock/tock && brew install tock-tools.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 16, 2020

I think I'm concluding that the thing to do at the moment is retain the riscv branch, but have it only be a single commit to modify TOCK_TARGETS. I would also update the readme to clarify that RISC-V compilation is optional, and that if you want to compile for riscv you need to install the toolchain and checkout the riscv branch.

I think it would be better to get these changes merged, while giving us more time to work on the toolchain problems.

@ppannuto
Copy link
Member

ppannuto commented Jun 16, 2020

I agree that these changes are good and shouldn't sit needlessly out-of-tree while we wait on toolchain stuff.

We could even be a bit smarter and just have TOCK_TARGETS conditionally include riscv targets based on the presence of the toolchain (roughly

ifeq ($(shell command -v riscv64-unknown-elf-gcc),)
TOCK_TARGETS ?= ...arm-only
else
TOCK_TARGETS ?= ...all
endif

should work).

@hudson-ayers
Copy link
Contributor

I agree that these changes are good and shouldn't sit needlessly out-of-tree while we wait on toolchain stuff.

We could even be a bit smarter and just have TOCK_TARGETS conditionally include riscv targets based on the presence of the toolchain (roughly

ifeq ($(shell command -v riscv64-unknown-elf-gcc),)
TOCK_TARGETS ?= ...arm-only
else
TOCK_TARGETS ?= ...all
endif

should work).

I agree that making risc-v compilation optional in the master branch is better than having it in a second branch that will have to be kept up to date with master somehow. But having the toolchain installed is not sufficient for it to actually work, and I don't think we should force users to uninstall their risc-v toolchain just to be able to build arm apps, all because their toolchain does not include newlib.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 16, 2020

I think it's important the out-of-master change is very small (and never grows). After this PR it would just be changing one make variable (which people will need to do anyway to support new risc-v boards with different flash and ram addresses).

But I think that is preferable to having a dynamic system where supporting users is first establishing what toolchains they have just to understand what our build system should be doing.

@hudson-ayers
Copy link
Contributor

Why not just require users to pass --riscv to any call to make to explicitly build apps for riscv. That seems a lot easier than checking out another branch. Or set an environment variable, so that it persists once you do it once.

@ppannuto
Copy link
Member

Oy. Didn't realize there was a newlib wildcard. Yeah, the single TARGETS patch feels good.

Or a guard on an environment variable, like if defined TOCK_LIBTOCKC_ENABLE_RISCV so that people doing development work don't have to keep this lingering one-line change around while they're working.

@alistair23
Copy link
Contributor

A single line Make change works for me, same with a command line argument of environment variable.

It kind of looks like Ubuntu is the only recent distro that doesn't support RV32 embedded libc, so this is really useful and simple for lots of people. Hopefully Ubuntu 20.10 fixes this and then we at least has a working version (although non-LTS versions don't make it to Travis).

Use `make RISCV=1` to build them.
@bradjc
Copy link
Contributor Author

bradjc commented Jun 17, 2020

I added make RISCV=1, which I think is better than the branch idea. It would be nice to include the build on travis, however, but I'm not sure how to best do that.

@hudson-ayers
Copy link
Contributor

I added make RISCV=1, which I think is better than the branch idea. It would be nice to include the build on travis, however, but I'm not sure how to best do that.

For CI, the options I see:

  1. transition travis to macos so we get the easy toolchain install via brew
  2. build newlib from source for ubuntu (this takes like 30 minutes, so don't love it as an option, but maybe we can figure out how to cache it?)
  3. add a github actions runner just to run macos tests that build a couple apps for risc-v

@bradjc
Copy link
Contributor Author

bradjc commented Jun 17, 2020

The riscv toolchain isn't the problem, the problem is the build all script doesn't include RISCV=1.

@ppannuto
Copy link
Member

Make picks up variables from the environment, i.e. $ RISCV=1 ./build_all.sh does the right thing.


For CI, probably a separate thing, but should we kick this repo over to github actions as well? That makes mac way less painful as a target.

@bradjc bradjc requested a review from ppannuto June 17, 2020 14:50
@hudson-ayers hudson-ayers changed the title Compile apps for RISC-V by default Compile apps for RISC-V with make flag Jun 17, 2020
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

bors r+

🎉

@bors
Copy link
Contributor

bors bot commented Jun 17, 2020

Build succeeded:

@bors bors bot merged commit 5d91991 into master Jun 17, 2020
@bors bors bot deleted the riscv-for-all branch June 17, 2020 17:17
tyler-potyondy pushed a commit to tyler-potyondy/libtock-c that referenced this pull request Mar 13, 2024
88: Compile apps for RISC-V with make flag r=ppannuto a=bradjc

This adds `rv32imac` binaries to TABs from libtock-c. These are compiled for specific flash and RAM addresses.

The main change for doing this is automatically generating the linker script when building for each architecture. This copies the default linker script and sets the addresses if needed. Different compilation versions are set in the Configuration.mk makefile.

This also compiles libraries for rv32imac, and updates some apps that use assembly.

TODO
------

- [x] fix crt0 header for riscv, need to take the stack size into account
- [x] make got just a copy
- [x] fix lst build for cortex-m

Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Co-authored-by: Adam H. Leventhal <ahl@oxide.computer>
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

6 participants