Skip to content

userspace: get rid of app section placeholders #13924

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 3 commits into from
Mar 4, 2019
Merged

userspace: get rid of app section placeholders #13924

merged 3 commits into from
Mar 4, 2019

Conversation

andrewboie
Copy link
Contributor

@andrewboie andrewboie commented Feb 28, 2019 •
edited
Loading

We used to leave byte-long placeholder symbols to ensure
that empty application memory sections did not cause
build errors.

Now we use some relatively portable inline assembly to
generate a symbol, but don't take up any extra space.

Adding a zero-sized partition to a domain will now
correctly fail an assertion.

Fixes: #13923

Signed-off-by: Andrew Boie [email protected]

@andrewboie andrewboie requested a review from nashif February 28, 2019 04:19
@andrewboie andrewboie mentioned this pull request Feb 28, 2019
Closed
@andrewboie andrewboie added the bug The issue is a bug, or the PR is fixing a bug label Feb 28, 2019
@andrewboie andrewboie added this to the v1.14.0 milestone Feb 28, 2019
@codecov-io
Copy link

codecov-io commented Feb 28, 2019 •
edited
Loading

Codecov Report

Merging #13924 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13924      +/-   ##
==========================================
+ Coverage   52.23%   52.33%   +0.09%     
==========================================
  Files         307      307              
  Lines       45448    45553     +105     
  Branches    10515    10561      +46     
==========================================
+ Hits        23742    23839      +97     
- Misses      16912    16914       +2     
- Partials     4794     4800       +6
Impacted Files Coverage Δ
subsys/testsuite/ztest/src/ztest.c 72.72% <ø> (+7.43%) ⬆️
lib/os/assert.c 0% <0%> (ø) ⬆️
subsys/net/l2/ethernet/arp.c 61.94% <0%> (+4.34%) ⬆️
include/net/ethernet.h 70.68% <0%> (+8.18%) ⬆️
subsys/testsuite/ztest/include/ztest_assert.h 77.77% <0%> (+38.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c8825f...d48b883. Read the comment docs.

@andrewboie andrewboie added the DNM This PR should not be merged (Do Not Merge) label Feb 28, 2019
ceolin
ceolin reviewed Feb 28, 2019
View reviewed changes
kernel/mem_domain.c
@@ -99,9 +99,10 @@ void k_mem_domain_init(struct k_mem_domain *domain, u8_t num_parts,

for (i = 0U; i < num_parts; i++) {
__ASSERT(parts[i] != NULL, "");
__ASSERT((parts[i]->start + parts[i]->size) >
parts[i]->start, "");

Copy link
Member

Choose a reason for hiding this comment

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

Questions, would this ASSERT never happens because the size would be at least 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that someone might not want to use the APPMEM partition macros, and just use the k_mem_partition and k_mem_domain directly? And, then, initialize a memory domain in that way? Then we should ASSERT that the size is not zero. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ioannisg I'm going to get rid of these changes to mem_domain.c in the next version of this patch

The goal of the next version of this patch will be to print a comprehensible error message if a 0-size partition is defined, instead of the mess of linker errors we get if the placeholder is removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this is done now.

ceolin
ceolin approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

Patch looks good. just another question, why just fail with that ASSERT when the partition is empty ?
It seems wrong create partitions that are no used, my guess is that these partitions can be used depending on which build options one is using.

@andrewboie
Copy link
Contributor Author

Patch looks good. just another question, why just fail with that ASSERT when the partition is empty ?
It seems wrong create partitions that are no used, my guess is that these partitions can be used depending on which build options one is using

I agree, and will be re-working this a bit.
Instead of just allowing zero-sized partitions, I'm going to look into whether we should instead just emit a build warning or error, with a comprehensible error message on what is going on.

Andrew Boie added 3 commits March 1, 2019 18:13
tests: userspace: remove unused partition
615cbfe 
No data was ever being put in part2.

Signed-off-by: Andrew Boie <[email protected]>
userspace: clarify memory domain assertions
51278ee 
Some text added to help explain what is going on.

Signed-off-by: Andrew Boie <[email protected]>
userspace: get rid of app section placeholders
d48b883 
We used to leave byte-long placeholder symbols to ensure
that empty application memory sections did not cause
build errors that were very difficult to understand.

Now we use some relatively portable inline assembly to
generate a symbol, but don't take up any extra space.

The malloc and libc partitions are now only instantiated
if there is some data to put in them.

Fixes: zephyrproject-rtos#13923

Signed-off-by: Andrew Boie <[email protected]>
@andrewboie andrewboie force-pushed the appmem-placeholders branch from ec6a4f3 to d48b883 Compare March 2, 2019 02:15
@andrewboie
Copy link
Contributor Author

@ioannisg @ceolin updated.

@andrewboie andrewboie requested review from nordic-krch, dcpleung and a user March 2, 2019 02:17
@andrewboie andrewboie added area: Memory Protection priority: medium Medium impact/importance bug and removed DNM This PR should not be merged (Do Not Merge) labels Mar 2, 2019
@andrewboie
Copy link
Contributor Author

One failing test traced to #14011

@andrewboie andrewboie merged commit 7707060 into zephyrproject-rtos:master Mar 4, 2019
@andrewboie andrewboie deleted the appmem-placeholders branch April 10, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif approved these changes

@ceolin ceolin ceolin approved these changes

@agross-oss agross-oss agross-oss approved these changes

@andyross andyross Awaiting requested review from andyross

@ioannisg ioannisg Awaiting requested review from ioannisg

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@dcpleung dcpleung Awaiting requested review from dcpleung

Assignees
No one assigned
Labels
area: Memory Protection bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Milestone
v1.14.0
Development

Successfully merging this pull request may close these issues.
6 participants
@andrewboie @codecov-io @nashif @ceolin @ioannisg @agross-oss