Skip to content

check if the requested memory is really available in _sbrk_r#1782

Merged
benpicco merged 3 commits intoRIOT-OS:masterfrom
benpicco:fix_malloc
Nov 5, 2014
Merged

check if the requested memory is really available in _sbrk_r#1782
benpicco merged 3 commits intoRIOT-OS:masterfrom
benpicco:fix_malloc

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Oct 8, 2014

Most RIOT ports do not check whether the memory returned by _sbrk_r actually exists, lpc1768 is the notable exception.

For it and all other newlib using ports, heap comes after the stack and occupies the remaining available RAM, so it's only necessary to check whether the heap pointer would exceed the physical range.

I've also re-added an old test that causes a hard fault without the patch (because it eventually tries to allocate memory that doesn't exist), but works as intended with the patch, even free works (due to newlib magic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming that length(ram) is a multiple of 8, try

. = ALIGN(8);
_sheap = .;
. = LENGTH(ram);
_eheap = .;

Then see the .map file whether the results are correct.

@miri64 miri64 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Oct 9, 2014
@miri64 miri64 assigned haukepetersen and unassigned Kijewski Oct 9, 2014
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What is the + 4 for?

@benpicco benpicco added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 9, 2014
@benpicco benpicco removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 14, 2014
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 17, 2014
@benpicco
Copy link
Copy Markdown
Contributor Author

ping?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tabs

@Kijewski
Copy link
Copy Markdown
Contributor

Safe for the style issues I guess this PR is fine.
But please double check the .map files if the address ranges are sane.

@benpicco benpicco force-pushed the fix_malloc branch 2 times, most recently from f915f9f to e6382e3 Compare October 23, 2014 11:19
@benpicco
Copy link
Copy Markdown
Contributor Author

Anything in the .map files I should watch out for?

@Kijewski
Copy link
Copy Markdown
Contributor

Only if the position of _sheap and _eheap looks sane.

@benpicco
Copy link
Copy Markdown
Contributor Author

            0x00000000200041e8                _sheap = .
            0x0000000020020000                _eheap = (ORIGIN (ram) + 0x20000)

looks sane imo

@Kijewski
Copy link
Copy Markdown
Contributor

Please rebase.

ACK probably

@benpicco
Copy link
Copy Markdown
Contributor Author

Should I revert 81dea36 and replace it with this more generic way, or should I just drop the cc2538 part from this patch?

@benpicco
Copy link
Copy Markdown
Contributor Author

like so - can you please test if this is allright @hexluthor ?

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Nov 3, 2014

Kicked Travis, because the errors seemed unrelated to the PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please undo.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

undone.

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Nov 3, 2014

Should I revert 81dea36 and replace it with this more generic way, or should I just drop the cc2538 part from this patch?

I'd say yes, use your variant for cc2538, too.

@hexluthor
Copy link
Copy Markdown
Contributor

2895002 seems to work ok on my cc2538dk. Feel free to clobber my sbrk() changes in cpu/cc2538.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Nov 3, 2014

so ack?

@hexluthor
Copy link
Copy Markdown
Contributor

ACK

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Nov 3, 2014

Needs fixup for spark-core.

@benpicco benpicco force-pushed the fix_malloc branch 2 times, most recently from 597b78b to f41d9ba Compare November 3, 2014 19:21
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Nov 3, 2014

Travis is happy 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

put a newline here

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Nov 5, 2014

ACK, put a newline there, and merge at will.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Nov 5, 2014

Ok

benpicco added a commit that referenced this pull request Nov 5, 2014
check if the requested memory is really available in _sbrk_r
@benpicco benpicco merged commit cedc588 into RIOT-OS:master Nov 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants