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/mips32r2: remove nomips16 attribute from _mips_handle_exception #11986

Merged
merged 1 commit into from
Sep 17, 2019

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Aug 9, 2019

Contribution description

A note says "The nomips16 attribute should not really be needed,
it works around a toolchain issue in 2016.05-03."

In fact, in 2018.09-03, the attribute leads to this error:

cpu/mips32r2_common/thread_arch.c:191:1: error: ‘_mips_handle_exception’ redeclared with conflicting ‘nomips16’ attributes

This this commit removes the attribute.

Testing procedure

Successful build and basic test on any mips32r2 board should suffice.

Issues/PRs references

Needed for RIOT-OS/riotdocker#76

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Aug 9, 2019
@kaspar030 kaspar030 requested a review from neiljay August 9, 2019 11:14
@MrKevinWeiss MrKevinWeiss added Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Process: release cycle Integration Process: The PR is connected to the release cycle (e.g. release notes) labels Aug 9, 2019
@kaspar030 kaspar030 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 19, 2019
@francois-berder
Copy link
Contributor

ACK. I compiled hello-world with GCC toolchain 2017.10-05 and this is necessary to compile any programs for MIPS CPU with toolchains newer than 2016.05-03.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Tested by @francois-berder
And the build was broken before anyway.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 17, 2019
@kaspar030
Copy link
Contributor Author

There's a logistics problem:

  • this PR doesn't build with current riotdocker compiler
  • update MIPS toolchain version riotdocker#76 doesn't pass CI because it breaks compiles with the attribute set (travis for riotdocker tests building the latest release)

@kaspar030
Copy link
Contributor Author

There's a logistics problem:

I propose:

If this is somewhat coordinated, mips would only be disabled for a couple of hours.

@kaspar030
Copy link
Contributor Author

Or, guard the attribute by "GNUC <= 4", then backport the fix. That should make the current RIOT CI pass. The riotdocker CI might need to be changed to test against the release branch, not the release tag.

@kaspar030
Copy link
Contributor Author

@francois-berder could you give this another shot?

@kaspar030
Copy link
Contributor Author

Ok, this is with a newer toolchain:

[kaspar@ng riot/examples/hello-world (mips_remove_nomips16)]$ RIOT_CI_BUILD=1 BOARD=pic32-wifire make clean all -j8
Building application "hello-world" for "pic32-wifire" with MCU "mips_pic32mz".

objcopy: --change-section-lma .gcc_except_table+0xffffffff80000000 never used
   text    data     bss     dec     hex filename
 111440    3017    5056  119513   1d2d9 /home/kaspar/src/riot/examples/hello-world/bin/pic32-wifire/hello-world.elf
[kaspar@ng riot/examples/hello-world (mips_remove_nomips16)]$ mips-mti-elf-gcc --version
mips-mti-elf-gcc (Codescape GNU Tools 2018.09-03 for MIPS MTI Bare Metal) 6.3.0
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[kaspar@ng riot/examples/hello-world (mips_remove_nomips16)]$ 

This is with the current docker image:

[kaspar@ng riot/examples/hello-world (mips_remove_nomips16)]$ BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 BOARD=pic32-wifire make clean all -j8
Launching build container using image "riot/riotbuild:latest".
docker run --rm -t -u "$(id -u)" \
    -v '/usr/share/zoneinfo/Europe/Berlin:/etc/localtime:ro' -v '/home/kaspar/src/riot:/data/riotbuild/riotbase' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles' -v /home/kaspar/.gitcache:/data/riotbuild/gitcache -e GIT_CACHE_DIR=/data/riotbuild/gitcache   \
    -e 'BOARD=pic32-wifire' -e 'RIOT_CI_BUILD=1' \
    -w '/data/riotbuild/riotbase/examples/hello-world/' \
    'riot/riotbuild:latest' make all  
Building application "hello-world" for "pic32-wifire" with MCU "mips_pic32mz".

objcopy: --change-section-lma .gcc_except_table+0xffffffff80000000 never used
   text    data     bss     dec     hex filename
 108828    2788    6024  117640   1cb88 /data/riotbuild/riotbase/examples/hello-world/bin/pic32-wifire/hello-world.elf
[kaspar@ng riot/examples/hello-world (mips_remove_nomips16)]$

So the guard is working. I'll squash, @benpicco could you re-ACK?

@benpicco
Copy link
Contributor

@kaspar030 the ACK is still valid, but so is the whitespace issue brought up by Travis - fix that and you should be good to go.

A note says "The nomips16 attribute should not really be needed,
it works around a toolchain issue in 2016.05-03."

In fact, in 2018.09-03, the attribute leads to this error:

```cpu/mips32r2_common/thread_arch.c:191:1: error: ‘_mips_handle_exception’ redeclared with conflicting ‘nomips16’ attributes```

This this commit removes the attribute.
@kaspar030
Copy link
Contributor Author

Backport provided in #12288

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 29, 2019
@kaspar030 kaspar030 deleted the mips_remove_nomips16 branch October 10, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Process: release cycle Integration Process: The PR is connected to the release cycle (e.g. release notes) Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants