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

rtos: rtx5: ARMCC5: move the variables in .bss.os section to ZI section #407

Merged

Conversation

TeroJaasko
Copy link
Contributor

@TeroJaasko TeroJaasko commented Aug 20, 2018

The thread stacks and other large variables in rtx_lib.c were directed
to .bss.os section, which puts them to zero initialized section in GCC,
but on ARMC5 they were put to data section which consumes precious ROM.

In reality the data section compression may make this a NOP change,
but it will at least make the total result statistics now more reliable.

Effects of this PR on the output of mbed compile of one application:

--8<--8<--
> --- memory_before_bss_change.txt	2018-06-18 16:45:51.928844313 +0300
> +++ memory_after_bss_change.txt	2018-06-18 16:57:27.753532437 +0300
> @@ -5,6 +5,9 @@
>  +----------------------------------------------------------+--------+-------+-------+
>  | Module                                                   |  .text | .data |  .bss |
>  +----------------------------------------------------------+--------+-------+-------+
> @@ -366,7 +369,7 @@
>  | mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_kernel.o  |    801 |   164 |     0 |
> -| mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.o     |    406 |  2117 |     0 |
> +| mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.o     |    406 |     1 |  2116 |
>  | mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_memory.o  |    260 |     0 |     0 |
> @@ -412,9 +415,9 @@
> -| Subtotals                                                | 163559 |  3380 | 15168 |
> +| Subtotals                                                | 163559 |  1264 | 17284 |
>  +----------------------------------------------------------+--------+-------+-------+
>  Total Static RAM memory (data + bss): 18548 bytes
> -Total Flash memory (text + data): 166939 bytes
> +Total Flash memory (text + data): 164823 bytes

The thread stacks and other large variables in rtx_lib.c were directed
to .bss.os section, which puts them to zero initialized section in GCC,
but on ARMC5 they were put to data section which consumes precious ROM.

In reality the data section compression may make this a NOP change,
but it will at least make the total result statistics now more reliable.

Effects of this PR on the output of mbed compile of one application:
--8<--8<--
> --- memory_before_bss_change.txt	2018-06-18 16:45:51.928844313 +0300
> +++ memory_after_bss_change.txt	2018-06-18 16:57:27.753532437 +0300
> @@ -5,6 +5,9 @@
>  +----------------------------------------------------------+--------+-------+-------+
>  | Module                                                   |  .text | .data |  .bss |
>  +----------------------------------------------------------+--------+-------+-------+
> @@ -366,7 +369,7 @@
>  | mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_kernel.o  |    801 |   164 |     0 |
> -| mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.o     |    406 |  2117 |     0 |
> +| mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.o     |    406 |     1 |  2116 |
>  | mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_memory.o  |    260 |     0 |     0 |
> @@ -412,9 +415,9 @@
> -| Subtotals                                                | 163559 |  3380 | 15168 |
> +| Subtotals                                                | 163559 |  1264 | 17284 |
>  +----------------------------------------------------------+--------+-------+-------+
>  Total Static RAM memory (data + bss): 18548 bytes
> -Total Flash memory (text + data): 166939 bytes
> +Total Flash memory (text + data): 164823 bytes
@TeroJaasko
Copy link
Contributor Author

This is a alternative implementation of #382.

@deepikabhavnani
Copy link

@RobertRostohar @JonatanAntoni - Please review

@JonatanAntoni
Copy link
Member

Thanks for adopting the fix. I am still a bit concerned that we need this only to get ArmCC working properly. But I think there is no way to get around.

@JonatanAntoni JonatanAntoni merged commit 0b4f11c into ARM-software:develop Sep 6, 2018
@JonatanAntoni JonatanAntoni added DONE and removed review labels Sep 6, 2018
@TeroJaasko TeroJaasko deleted the armcc_bss_os_fix_take_two branch September 6, 2018 12:15
@JonatanAntoni JonatanAntoni added review and removed DONE labels Sep 7, 2018
@JonatanAntoni
Copy link
Member

I needed to revert the merge, sorry. We came up with additional concerns because providing __SECTION_ZERO_INIT macro might be misleading. In fact using this macro with compilers others than AC5 will not effectively force the section to be zero initialized. This can only be achieved by adhering to the naming convention, i.e. .bbs section name prefix.

Hence we are back to a local solution to be added to rtx_lib.c:

#ifdef   __CC_ARM
#define  ZERO_INIT __attribute__((zero_init))
#else
#define  ZERO_INIT
#endif

__attribute__((section(".bss.os"))) ZERO_INIT;

Your feedback is appreciated.

@JonatanAntoni
Copy link
Member

@TeroJaasko, @deepikabhavnani: My I ask you to comment on the updated solution proposal, please?

@deepikabhavnani
Copy link

@JonatanAntoni - Agree to your point that __SECTION_ZERO_INIT might be misleading, we can have it for ARM 5 compiler only.

@JonatanAntoni
Copy link
Member

Thanks @deepikabhavnani. I guess @RobertRostohar will provide an alternative implementation, soon.

@TeroJaasko
Copy link
Contributor Author

@JonatanAntoni I'm a bit puzzled. There was #382 with the local hack for ARMC5 only, but that was rejected because it was a local hack for ARMC5 only and people wanted to have the same macro for all compilers. But as both approaches do have the same end result I don't care how it gets eventually done:)

@JonatanAntoni
Copy link
Member

Hi @TeroJaasko,

Yes, sorry for that confusion. Actually we have made a turn during our discussion back towards your original proposal. It looked like a good idea to have such a macro in a central place instead of adding "local hacks" at the first glance.

Looking to this from another perspective revealed that SECTION_ZERO_INIT does not actually do what it proposes on any compiler other than AC5. In other words: Using SECTION_ZERO_INIT("my_section") would not lead to a zero-initialized section on AC6 for instance. Hence we rated the global solution to be counterintuitive.

The local solution sketched above it not really intuitive either, i.e. __attribute__((section("my_section))) ZERO_INIT wouldn't be zero-initialized on AC6. But by keeping the scope at minimum it should not lead to a wider confusion, hopefully.

Does this make sense for you?

Cheers,
Jonatan

@JonatanAntoni
Copy link
Member

Fixed in 68e535d

JonatanAntoni added a commit that referenced this pull request Sep 12, 2018
This change breaks exiting projects which are using the sections for user-specified control blocks.

This reverts commit 027464c.

Change-Id: I938824a8afe2aa0c887a5f1ff7c643ed7ef18a49
@JonatanAntoni
Copy link
Member

Unfortunately we needed to revert that fix. It breaks existing projects.

@JonatanAntoni JonatanAntoni added review and removed DONE labels Sep 12, 2018
@TeroJaasko
Copy link
Contributor Author

@JonatanAntoni care to open a bit where something did break and how? I did verify the original PR's on Mbed OS with ARMC5, ARMC6, IAR and GCC so a breakage is bit of a surprise.

@JonatanAntoni
Copy link
Member

@TeroJaasko: It breaks exiting projects where the user manually added statically allocated control block to the named sections. This applies for our own middleware components for instance. Hence adding this fix to CMSIS we would need to update all the dependent software components. Unfortunately updating the software components would lead to a new version that cannot be used together with an older CMSIS release. That's not a situation we can effort at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants