Skip to content

Fix memory allocation on STM32L4 devices #7950

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
Merged
merged 2 commits into from
Sep 8, 2018
Merged

Fix memory allocation on STM32L4 devices #7950

merged 2 commits into from
Sep 8, 2018

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Aug 31, 2018

Description

Depending on initial size allocated on STM32L4 devices with TWO_RAM_REGIONS set a crash may occur. This is because there is a mismatch between the size newlib is expecting and the size actually returned by _sbrk. This is because the STM32L4 implementation of _sbrk is performing alignment internally.

This patch fixes this problem by removing the code in __wrap__sbrk which performs the alignment.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@c1728p9
Fix memory allocation on STM32L4 devices
e2d003a 
Depending on initial size allocated on STM32L4 devices with
TWO_RAM_REGIONS  set a crash may occur. This is because there is a
mismatch between the size newlib is expecting and the size actually
returned by _sbrk. This is because the STM32L4 implementation of _sbrk
is performing alignment internally.

This patch fixes this problem by removing the code in __wrap__sbrk
which performs the alignment.
@c1728p9 c1728p9 mentioned this pull request Aug 31, 2018
Closed
@c1728p9 c1728p9 requested review from cmonr and LMESTM August 31, 2018 23:36
@0xc0170 0xc0170 requested a review from a team September 1, 2018 08:38
adustm
adustm reviewed Sep 3, 2018
View reviewed changes
targets/TARGET_STM/TARGET_STM32L4/l4_retarget.c Outdated
uint32_t heap_ind_old = STM32L4_ALIGN_UP(heap_ind, STM32L4_HEAP_ALIGN);
uint32_t heap_ind_new = STM32L4_ALIGN_UP(heap_ind_old + incr, STM32L4_HEAP_ALIGN);
uint32_t heap_ind_old = heap_ind;
uint32_t heap_ind_new = heap_ind_old + incr;

if (heap_ind_new > &__mbed_krbs_start) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, could we take the opportunity of removing a compilation warning here ?

[Warning] l4_retarget.c@54,22: comparison between pointer and integer
[DEBUG] Return: 0
[DEBUG] Output: .\targets\TARGET_STM\TARGET_STM32L4\l4_retarget.c: In function '__wrap__sbrk':
[DEBUG] Output: .\targets\TARGET_STM\TARGET_STM32L4\l4_retarget.c:54:22: warning: comparison between pointer and integer
[DEBUG] Output:      if (heap_ind_new > &__mbed_krbs_start) {
[DEBUG] Output:                       ^

With

-    if (heap_ind_new > &__mbed_krbs_start) {
+    if (heap_ind_new > (uint32_t)&__mbed_krbs_start) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this warning 💯

@adustm
Copy link
Member

adustm commented Sep 3, 2018

cc @betzw (I think you might appreciate this ;) )

Hi @c1728p9
Many thanks for this modification. I tested it ok for #7867
Kind regards

@betzw
Copy link
Contributor

betzw commented Sep 3, 2018

@adustm 😄

@MateuszMaz
Copy link
Contributor

Hi @c1728p9 , as you requested I tested it. It works for l475 and l476. I also checked all the tests that mbed-os and ci test shield provides - everything is ok about this fix, no regression anywhere

@c1728p9
Fix comparison warning on STM32L4 devices
e084865 
Cast the pointer used in l4_retarget to uint32_t before comparing it
to fix the warning:
"comparison between pointer and integer"
@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 6, 2018

Fixed warnings as well. @cmonr can you make sure this gets into the next RC?

@cmonr
Copy link
Contributor

cmonr commented Sep 6, 2018

Marked for RC2 for now, but this doesn't look like it was discovered during OOB.
@adbridge Thoughts?

@adbridge
Copy link
Contributor

adbridge commented Sep 6, 2018

@cmonr We should only be bringing in essential fixes for RC2. As this has not been found to be an issue so far we should probably push it to 5.10.1

@jeromecoutant
Copy link
Collaborator

this has not been found to be an issue so far

This is maybe because there is no STM32L4 devices with TWO_RAM_REGIONS in the CI :-)

@adustm
Copy link
Member

adustm commented Sep 7, 2018

Hello @adbridge
Every STM32L476 / STM32L475 have issues on the current release about memory mapping.

Please refer to #7867
I'm wondering wether this issue is related or not
https://os.mbed.com/questions/82162/Cannot-get-SD-card-working-with-NUCLEO-L/

Kind regards

@adbridge
Copy link
Contributor

adbridge commented Sep 7, 2018

Moving this back to RC2 so that we can get the fix into 5.10 as it looks like this has a bigger impact than originally thought.

@adbridge
Copy link
Contributor

adbridge commented Sep 7, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2018

Build : SUCCESS

Build number : 3030
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7950/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2018

Test : FAILURE

Build number : 2801
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7950/2801

@cmonr
Copy link
Contributor

cmonr commented Sep 7, 2018

Test failure does not appear to be related to PR.

/morph test

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2018

Test : FAILURE

Build number : 2802
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7950/2802

@cmonr
Copy link
Contributor

cmonr commented Sep 7, 2018

Test failure does not appear to be related to PR.
/morph test

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2018

Exporter Build : FAILURE

Build number : 2647
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7950/

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2018

Test : SUCCESS

Build number : 2804
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7950/2804

@cmonr
Copy link
Contributor

cmonr commented Sep 7, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Sep 8, 2018

Exporter Build : SUCCESS

Build number : 2651
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7950/

@cmonr cmonr merged commit d311a96 into ARMmbed:master Sep 8, 2018
@0xc0170 0xc0170 removed the needs: CI label Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@adustm adustm adustm left review comments

@LMESTM LMESTM LMESTM approved these changes

@cmonr cmonr Awaiting requested review from cmonr

Assignees
No one assigned
Labels
release-version: 5.10.0-rc2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@c1728p9 @adustm @betzw @MateuszMaz @cmonr @adbridge @jeromecoutant @mbed-ci @0xc0170 @LMESTM