cpu/mips: Integrate better with RIOT newlib layer (v2) by neiljay · Pull Request #6892 · RIOT-OS/RIOT

neiljay · 2017-04-11T13:23:34Z

This is an update to #6639 as contributed by @gebart .
Depends on ~~#6670~~

Copied the relevant parts of mipshal.mk from the toolchain
Cleaned up CFLAGS and more in Makefile.include
Added syscalls implementation which can be used to interface with the UHI system, in reference to mips: Build system is not aligned with other platforms #6588 (comment)
Added a note on why -std=gnu99 is necessary for this CPU presently, should be removed once the toolchain is updated to a more recent newlib version.
Various other makefile clean-up for MIPS

neiljay · 2017-04-20T15:55:56Z

Ok to squash this ?

- Copied the relevant parts of mipshal.mk from the toolchain - Cleaned up CFLAGS and more in Makefile.include - Added empty syscalls implementation which can be used to interface with the UHI system - Added a note on why -std=gnu99 is necessary for this CPU presently

Some headers in the tool chain cause compilation errors with Clang in assembly mode.

Causes compilation error on Clang

neiljay · 2017-06-19T13:48:13Z

@gebart can I get a review, ta?

jnohlgard

See my comments inline

jnohlgard · 2017-06-19T15:03:18Z

cpu/mips32r2_common/Makefile.include

+else
+  #Use RIOT to handle syscalls (VFS)
+  export LINKFLAGS += -Tuhi32.ld
+  export USEMODULE += vfs


It should not be necessary to add VFS for the default sys calls. There's a light weight implementation which supports only read and write of stdin/stdout and no open, close etc. That should be used if USEMODULE += VFS is not specified.

jnohlgard · 2017-06-19T15:53:34Z

cpu/mips32r2_common/Makefile.include

+  export USEMODULE += newlib_syscalls_mips_uhi
+else
+  #Use RIOT to handle syscalls (VFS)
+  export LINKFLAGS += -Tuhi32.ld


Is uhi32.ld the correct linker script for the non UHI build as well?

Yes, its the only one we have for a default mips32 implementation, it is copied from the toolchains in-built linker scripts and follows its naming convention.

jnohlgard · 2017-06-19T16:00:04Z

cpu/mips32r2_common/newlib_syscalls_mips_uhi/syscalls.c

+*/
+int _close_r(struct _reent *r, int fd)
+{
+    int res = close(fd);


Why not simply return close(x)? (same goes for the other wrapper functions as well)

Easier to view in a GUI debugger, compiler will optimise away anyway.

jnohlgard · 2017-06-19T16:00:38Z

cpu/mips32r2_common/newlib_syscalls_mips_uhi/syscalls.c

+* @param r TODO
+* @param fd TODO
+*
+* @return TODO


Missing docs

jnohlgard · 2017-06-19T16:00:48Z

cpu/mips32r2_common/newlib_syscalls_mips_uhi/syscalls.c

+* @param[in] pid the pid to send to
+* @param[in] sig the signal to send
+*
+* @return TODO


Missing docs

jnohlgard · 2017-06-19T16:01:36Z

cpu/mips_pic32mx/Makefile.include

+  export USEMODULE += newlib_syscalls_mips_uhi
+else
+  #Use RIOT to handle syscalls (VFS)
+  export USEMODULE += vfs


Should not be necessary

jnohlgard · 2017-06-19T16:01:54Z

cpu/mips_pic32mz/Makefile.include

+  export USEMODULE += newlib_syscalls_mips_uhi
+else
+  #Use RIOT to handle syscalls (VFS)
+  export USEMODULE += vfs


Should not be necessary

jnohlgard · 2017-06-19T16:10:27Z

makefiles/arch/mips.inc.mk

+export CFLAGS += -std=gnu99
+export CFLAGS_CPU   = -EL -mabi=$(ABI)
+# Why not -fdata-sections?? /JN
 export CFLAGS_LINK  = -ffunction-sections -fno-builtin -fshort-enums #-fdata-sections


Why not -fdata-sections?

I was having problems with the linker stripping out the pic32 configuration registers (which have to reside at a specific place in the final flash image), but this was actually due to a missing KEEP() attribute in the linker script, so this can be put back now.

jnohlgard · 2017-06-19T16:17:20Z

makefiles/libc/newlib.mk

+ifeq ($(TARGET_ARCH),mips-mti-elf)
+  export LINKFLAGS += -lc
+else
+  export LINKFLAGS += -lc -lnosys


I think -lnosys might be unused on all platforms now with proper syscall implementations. Could you try unconditionally removing -lnosys and see what Murdock says?

ok will give this a try.

hmm, I thought this had worked, but must have missed the error message.
jerryscript is not compiling on arm targets without nosys:

/usr/bin/../lib/gcc/arm-none-eabi/6.3.1/../../../../arm-none-eabi/lib/thumb/v6-m/libc_nano.a(lib_a-gettimeofdayr.o): In function _gettimeofday_r': gettimeofdayr.c:(.text._gettimeofday_r+0xe): undefined reference to _gettimeofday'

I think you have found a bug in our newlib syscall implementation

We can either use the UHI syscall implementation or the RIOT default one (targeted at VFS, uart-stdio will be added once uart rx is added to pic32).

neiljay · 2017-07-03T15:22:09Z

@gebart ok now ?

jnohlgard

Looks good, I can't test the binaries since I don't have a mips board. I assume it has been tested on your side?
Also see my minor comment on cppcheck suppression.

jnohlgard · 2017-07-04T05:50:22Z

cpu/mips32r2_common/newlib_syscalls_mips_uhi/syscalls.c

+void _exit(int n)
+{
+    exit(n);
+    /* cppcheck-suppress unreachableCode */


All cppcheck suppressions should have an accompanying comment describing why it is a false positive.

Ok, I would hope this one is fairly obvious why the code is unreachable...

neiljay · 2017-07-04T11:35:00Z

Yes, I've tested with default syscalls and UHI but not VFS, It would be nice if there was a VFS example in 'examples', a simple ramfs + pre-baked filesystem with a couple of text files you could cat in a console ?

jnohlgard · 2017-07-04T14:51:42Z

@neiljay it would be fairly easy to add a constfs file system to the default example. See the unittests for VFS which use constfs to provide a test file system without any external dependencies. That should be a separate PR though.

neiljay · 2017-07-12T09:18:11Z

@gebart, Do you want me to make that last minor change or can we merge as-is ?

jnohlgard · 2017-07-12T12:13:34Z

@neiljay add a comment for the cppcheck suppression, other than that this seem ready to merge. The constfs stuff should be a separate PR of you want to add it.

jnohlgard

ACK

neiljay changed the title ~~Pr/mips newlib v2~~ cpu/mips: Integrate better with RIOT newlib layer (v2) Apr 11, 2017

neiljay force-pushed the pr/mips-newlib-v2 branch from 74e1582 to f7afe4f Compare April 11, 2017 13:30

miri64 assigned jnohlgard, kaspar030 and kYc0o Apr 11, 2017

jnohlgard mentioned this pull request Apr 11, 2017

cpu/mips: Integrate better with RIOT newlib layer #6639

Closed

neiljay force-pushed the pr/mips-newlib-v2 branch from 397cac2 to 348d9da Compare June 2, 2017 09:12

Joakim Nohlgård added 3 commits June 2, 2017 10:24

cpu/mips32r2_common: Use GCC for assembler files if TOOLCHAIN=llvm

b59a9cb

Some headers in the tool chain cause compilation errors with Clang in assembly mode.

cpu/mips32r2_common: ifdef __attribute__((optimize))

91d930c

Causes compilation error on Clang

neiljay force-pushed the pr/mips-newlib-v2 branch from 348d9da to 309b551 Compare June 2, 2017 09:27

neiljay requested a review from jnohlgard June 2, 2017 10:48

jnohlgard requested changes Jun 19, 2017

View reviewed changes

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

cpu/mips32r2: Add UHI sycalls implementation.

9d736dc

neiljay force-pushed the pr/mips-newlib-v2 branch from 392cb44 to cf81c22 Compare June 28, 2017 11:16

Neil Jones added 3 commits June 28, 2017 12:55

makefiles: Don't link libnosys for mips and put back -fdata-sections

6c54360

sys/newlib: Allow building default syscall implementation for mips

7b824fe

We can either use the UHI syscall implementation or the RIOT default one (targeted at VFS, uart-stdio will be added once uart rx is added to pic32).

mips: Clean-up makefiles

af70c08

neiljay force-pushed the pr/mips-newlib-v2 branch from cf81c22 to e191371 Compare June 28, 2017 12:00

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

jnohlgard reviewed Jul 4, 2017

View reviewed changes

neiljay force-pushed the pr/mips-newlib-v2 branch from e191371 to 0b8ab6c Compare July 12, 2017 14:56

mips: syscalls: supress ccpcheck error

54eb49a

neiljay force-pushed the pr/mips-newlib-v2 branch from 0b8ab6c to 54eb49a Compare July 17, 2017 16:18

jnohlgard approved these changes Jul 19, 2017

View reviewed changes

jnohlgard merged commit da4f2f6 into RIOT-OS:master Jul 19, 2017

neiljay mentioned this pull request Nov 16, 2017

mips: Build system is not aligned with other platforms #6588

Closed

4 tasks

kYc0o mentioned this pull request Nov 22, 2017

pic32-wifire: race-condition when linking in concurrent build #7753

Closed

Conversation

neiljay commented Apr 11, 2017 • edited by jnohlgard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljay commented Apr 20, 2017

Uh oh!

neiljay commented Jun 19, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljay commented Jul 3, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljay commented Jul 4, 2017

Uh oh!

jnohlgard commented Jul 4, 2017

Uh oh!

neiljay commented Jul 12, 2017

Uh oh!

jnohlgard commented Jul 12, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neiljay commented Apr 11, 2017 •

edited by jnohlgard

Loading