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.

Sign up for GitHub

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

Jump to bottom

riscv: fix tests/pkg_wolfssl and tests/lwip_gnrc_udp when building with default options #12502

Merged
merged 2 commits into from
Oct 21, 2019
Merged

riscv: fix tests/pkg_wolfssl and tests/lwip_gnrc_udp when building with default options #12502

merged 2 commits into from
Oct 21, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Oct 18, 2019 •
edited by miri64
Loading

Contribution description

This PR is an attempt to get a working build on hifive1/hifive1b boards with tests/pkg_wolfssl and tests/lwip_gnrc_udp applications. The failures were reported in RIOT-OS/Release-Specs#140 (comment) and RIOT-OS/Release-Specs#140 (comment).

For the wolfssl test application, removing the optimization flash from the debug flags of the toolchain fixed the issue.
For the lwip_gnrc_udp, I'm less sure of the fix because I have to admit I don't really understand the error message. Disabling any GCC optimizations on the problematic functions fix the build issue. But maybe there's an option that would be better ? Any better suggestion is welcome (I also set the RFC label because of this)

Since this PR is touching the RISV toolchain options, it may have other impacts I didn't think of.

Testing procedure

At least the 2 related applications should still work. But better test all applications on hifive1. (the fixes are more localized now /@miri64)

Issues/PRs references

Reported in RIOT-OS/Release-Specs#140

@aabadie aabadie added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Oct 18, 2019
@aabadie aabadie mentioned this pull request Oct 18, 2019
Closed
80 tasks
@miri64
Copy link
Member

miri64 commented Oct 18, 2019

For the lwip_gnrc_udp, I'm less sure of the fix because I have to admit I don't really understand the error message. Disabling any GCC optimizations on the problematic functions fix the build issue. But maybe there's an option that would be better ? Any better suggestion is welcome (I also set the RFC label because of this)

This is not an optimization issue I think. Let me try something.

@miri64
Copy link
Member

miri64 commented Oct 18, 2019

This is not an optimization issue I think. Let me try something.

(IPv6 addresses in lwIP are not just represented by their actual bytes as they are typically in RIOT, but also contain a bunch of lwIP-specific meta information. So I think this is just a byte-width issue)

@miri64
Copy link
Member

miri64 commented Oct 18, 2019

See aabadie#14

@aabadie aabadie force-pushed the pr/riscv_fix_build branch from 9a51fa0 to e67b9d5 Compare October 18, 2019 19:01
@aabadie aabadie added CI: run tests If set, CI server will run tests on hardware for the labeled PR 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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 18, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Oct 18, 2019

The build passed on Murdock so I squashed and added you (@miri64) as co-author of the commit (since you found the solution).
Retriggered Murdock with run tests.

@miri64 miri64 added this to the Release 2019.10 milestone Oct 19, 2019
@miri64 miri64 requested a review from fjmolinas October 19, 2019 10:48
@miri64
Copy link
Member

miri64 commented Oct 19, 2019

I'd ACK, but I provided ~50% of the code, so please, somebody else review it.

miri64
miri64 reviewed Oct 19, 2019
View reviewed changes
makefiles/arch/riscv.inc.mk Outdated Show resolved Hide resolved
miri64
miri64 reviewed Oct 19, 2019
View reviewed changes
makefiles/arch/riscv.inc.mk Outdated Show resolved Hide resolved
@aabadie aabadie force-pushed the pr/riscv_fix_build branch from e67b9d5 to b9a380c Compare October 20, 2019 09:45
@aabadie
Copy link
Contributor Author

aabadie commented Oct 20, 2019

@miri64, I reverted the opt flags changes for riscv and went for your suggestion of disabling maybe-uninitialized when building wolfcrypt.
Regarding the default optimization used with riscv (-Og), there's no mention/comment about that in the original PR. So it's not clear where this value comes from. I think setting -Os should be default (optimized for size), it would make more sense for our constrained devices case.
This could be done in a follow-up PR if agreed.

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 20, 2019
@miri64
Copy link
Member

miri64 commented Oct 20, 2019

I'd ACK, but I provided ~50% of the code, so please, somebody else review it.

@benpicco maybe?

@miri64 miri64 mentioned this pull request Oct 20, 2019
Merged
aabadie and others added 2 commits October 21, 2019 08:09
@aabadie @miri64
tests/lwip_sock_udp: fix sizeof address param in mempcy
300c3ab 
Co-authored-by: Martine Lenders <m.lenders@fu-berlin.de>
@aabadie
pkg/wolfssl: disable maybe-uninitialized warning with wolfcrypt
02ff487 
Only when building with riscv toolchain, because the default optimization used can lead to this problem.
@aabadie aabadie force-pushed the pr/riscv_fix_build branch from b9a380c to 02ff487 Compare October 21, 2019 06:10
@aabadie
Copy link
Contributor Author

aabadie commented Oct 21, 2019

please apply @benpicco's change suggestion.

Done and directly squashed after verification that the test still passes on native (and still builds for hifive1 board).

benpicco
benpicco previously requested changes Oct 21, 2019
View reviewed changes
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.

Fixes how the parameters for memcpy are derived, but the values don't change.
Also suppresses a warning - no functional changes.

@benpicco benpicco dismissed their stale review October 21, 2019 08:34

Fat fingers

benpicco
benpicco approved these changes Oct 21, 2019
View reviewed changes
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.

Fixes how the parameters for memcpy are derived, but the values don't change.
Also suppresses a warning - no functional changes.

@miri64 miri64 merged commit 3564546 into RIOT-OS:master Oct 21, 2019
@miri64
Copy link
Member

miri64 commented Oct 21, 2019

Thanks @benpicco. Please provide a backport @aabadie.

@aabadie aabadie mentioned this pull request Oct 21, 2019
Merged
@aabadie aabadie deleted the pr/riscv_fix_build branch October 21, 2019 08:39
@aabadie
Copy link
Contributor Author

aabadie commented Oct 21, 2019

Done #12524 :)

@kaspar030 kaspar030 mentioned this pull request Oct 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@miri64 miri64 miri64 left review comments

@benpicco benpicco benpicco approved these changes

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
4 participants
@aabadie @miri64 @benpicco @kaspar030