Arch arm cortex m text section fix #21874

ioannisg · 2020-01-13T23:00:24Z

Rename TEXT_SECTION_NAME_2 to text (TEXT_SECTION_NAME)

Addresses #21650 for Cortex-M

Additionally, fix some wrong pre-processor directives' indentation. [trivial]

andrewboie

Not thrilled with the indentation changed, but not enough to -1.

Other patch LGTM, assuming sanitycheck doesn't throw errors about extra sections and this doesn't mess with MPU configuration.

include/arch/arm/aarch32/cortex_m/scripts/linker.ld

one more thought

andrewboie · 2020-01-13T23:42:00Z

Could we just reverse the order of the text and data sections? and coalesce the vector table and ARM exception unwinding sections into rodata?

ioannisg · 2020-01-14T09:38:47Z

Could we just reverse the order of the text and data sections? and coalesce the vector table and ARM exception unwinding sections into rodata?

In principle this could be done @andrewboie ; I am just wondering why we would like to do this for ARM Cortex-M; apparently there's a similar sructure in other ARCHEs; text section comes before rodata....

andrewboie · 2020-01-14T18:43:53Z

apparently there's a similar sructure in other ARCHEs; text section comes before rodata...

There's no requirement to what order they appear in. I reordered some data sections on 32-bit x86 to solve a technical problem.

ioannisg · 2020-01-14T21:12:00Z

apparently there's a similar sructure in other ARCHEs; text section comes before rodata...

There's no requirement to what order they appear in. I reordered some data sections on 32-bit x86 to solve a technical problem.

I understand. But what is the problem here, with the current ordering of the sections? If reordering does an improvement, I am up for it. :)

andrewboie · 2020-01-14T21:25:00Z

I'm not saying there's a problem, but I think reversing the sections is simpler

ioannisg · 2020-01-15T22:18:16Z

@wentongwu could you please review this patch (the one that renames the .text section) and see if this will work with Code Relocation?

andrewboie · 2020-01-15T22:23:06Z

@wentongwu can you take a look at this?

switching the rodata and text areas had side effects, this approach is OK with me

oyvindronningstad

The #include <snippets-text-start.ld> is now no longer in the text section, so you should move it. Unfortunately, this means moving the contents of aarch32/vector_table.ld, text_section_offset.ld (keep this file still for riscv and x86), and cc32xx_debug.ld back into the new vector table section.

oyvindronningstad · 2020-01-17T09:16:14Z

include/arch/arm/aarch32/cortex_m/scripts/linker.ld

@@ -135,7 +135,8 @@ SECTIONS

 	_image_rom_start = ROM_ADDR;

-    SECTION_PROLOGUE(_TEXT_SECTION_NAME,,)
+    /* The exception vector table is located at the beginning of ROM */
+    SECTION_PROLOGUE(vector_table,,)


riscv has the same scheme, but calls the section "vector", maybe align on that name?

I think the name is good; it is actually the ARM Cortex-M exception vector table; maybe we could rename the Risc-V Section instead, but that's another story.

oyvindronningstad · 2020-01-17T13:39:25Z

I'm working on this PR.

vanti

I tried this on cc3220sf_launchxl, and got a linker error:

$ west build -b cc3220sf_launchxl
-- west build: build configuration:
       source directory: /home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get
       build directory: /home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build
       BOARD: cc3220sf_launchxl (origin: CMakeCache.txt)
-- west build: building application
[1/6] Linking C executable zephyr/zephyr_prebuilt.elf
FAILED: zephyr/zephyr_prebuilt.elf 
: && ccache /home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc    zephyr/CMakeFiles/zephyr_prebuilt.dir/misc/empty_file.c.obj  -o zephyr/zephyr_prebuilt.elf  -Wl,-T zephyr/linker.cmd -Wl,-Map=/home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build/zephyr/zephyr_prebuilt.map -Wl,--whole-archive app/libapp.a zephyr/libzephyr.a zephyr/arch/common/libarch__common.a zephyr/arch/arch/arm/core/aarch32/libarch__arm__core__aarch32.a zephyr/arch/arch/arm/core/aarch32/cortex_m/libarch__arm__core__aarch32__cortex_m.a zephyr/lib/libc/newlib/liblib__libc__newlib.a zephyr/lib/posix/liblib__posix.a zephyr/boards/arm/cc3220sf_launchxl/libboards__arm__cc3220sf_launchxl.a zephyr/subsys/net/libsubsys__net.a zephyr/subsys/net/l2/wifi/libsubsys__net__l2__wifi.a zephyr/subsys/net/ip/libsubsys__net__ip.a zephyr/drivers/gpio/libdrivers__gpio.a zephyr/drivers/serial/libdrivers__serial.a modules/ti/simplelink/source/ti/devices/cc32xx/lib..__modules__hal__ti__simplelink__source__ti__devices__cc32xx.a modules/ti/simplelink/lib..__modules__hal__ti__simplelink.a -Wl,--no-whole-archive zephyr/kernel/libkernel.a zephyr/CMakeFiles/offsets.dir/arch/arm/core/offsets/offsets.c.obj -L"/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/thumb/v7e-m/nofp" -L/home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build/zephyr -lgcc -Wl,--print-memory-usage -mthumb -mcpu=cortex-m4 -Wl,--gc-sections -Wl,--build-id=none -Wl,--sort-common=descending -Wl,--sort-section=alignment -Wl,-u,_OffsetAbsSyms -Wl,-u,_ConfigAbsSyms -nostdlib -static -no-pie -Wl,-X -Wl,-N -Wl,--orphan-handling=warn -mabi=aapcs -march=armv7e-m -lm -lc -lgcc -L"/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/arm-zephyr-eabi"/lib/thumb/v7e-m/nofp && :
/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/../../../../arm-zephyr-eabi/bin/ld:zephyr/linker.cmd:45: syntax error
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

I have attached linker.cmd before and after the change.
linker.cmd.after.txt
linker.cmd.before.txt

We should not use indentation for pre-processor directives. This commit fixes the indentation in the ARM Cortex-M linker script. Signed-off-by: Ioannis Glaropoulos <[email protected]>

oyvindronningstad · 2020-01-21T09:37:49Z

I tried this on cc3220sf_launchxl, and got a linker error:

$ west build -b cc3220sf_launchxl
-- west build: build configuration:
       source directory: /home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get
       build directory: /home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build
       BOARD: cc3220sf_launchxl (origin: CMakeCache.txt)
-- west build: building application
[1/6] Linking C executable zephyr/zephyr_prebuilt.elf
FAILED: zephyr/zephyr_prebuilt.elf 
: && ccache /home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc    zephyr/CMakeFiles/zephyr_prebuilt.dir/misc/empty_file.c.obj  -o zephyr/zephyr_prebuilt.elf  -Wl,-T zephyr/linker.cmd -Wl,-Map=/home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build/zephyr/zephyr_prebuilt.map -Wl,--whole-archive app/libapp.a zephyr/libzephyr.a zephyr/arch/common/libarch__common.a zephyr/arch/arch/arm/core/aarch32/libarch__arm__core__aarch32.a zephyr/arch/arch/arm/core/aarch32/cortex_m/libarch__arm__core__aarch32__cortex_m.a zephyr/lib/libc/newlib/liblib__libc__newlib.a zephyr/lib/posix/liblib__posix.a zephyr/boards/arm/cc3220sf_launchxl/libboards__arm__cc3220sf_launchxl.a zephyr/subsys/net/libsubsys__net.a zephyr/subsys/net/l2/wifi/libsubsys__net__l2__wifi.a zephyr/subsys/net/ip/libsubsys__net__ip.a zephyr/drivers/gpio/libdrivers__gpio.a zephyr/drivers/serial/libdrivers__serial.a modules/ti/simplelink/source/ti/devices/cc32xx/lib..__modules__hal__ti__simplelink__source__ti__devices__cc32xx.a modules/ti/simplelink/lib..__modules__hal__ti__simplelink.a -Wl,--no-whole-archive zephyr/kernel/libkernel.a zephyr/CMakeFiles/offsets.dir/arch/arm/core/offsets/offsets.c.obj -L"/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/thumb/v7e-m/nofp" -L/home/local/zephyrproject-work/zephyr/samples/net/sockets/http_get/build/zephyr -lgcc -Wl,--print-memory-usage -mthumb -mcpu=cortex-m4 -Wl,--gc-sections -Wl,--build-id=none -Wl,--sort-common=descending -Wl,--sort-section=alignment -Wl,-u,_OffsetAbsSyms -Wl,-u,_ConfigAbsSyms -nostdlib -static -no-pie -Wl,-X -Wl,-N -Wl,--orphan-handling=warn -mabi=aapcs -march=armv7e-m -lm -lc -lgcc -L"/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/arm-zephyr-eabi"/lib/thumb/v7e-m/nofp && :
/home/local/bin/zephyr-sdk-0.10.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/../../../../arm-zephyr-eabi/bin/ld:zephyr/linker.cmd:45: syntax error
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

I have attached linker.cmd before and after the change.
linker.cmd.after.txt
linker.cmd.before.txt

Thanks, @vanti. I fixed it and tested locally, it compiles cleanly now.

vanti · 2020-01-21T19:22:36Z

Thanks, @vanti. I fixed it and tested locally, it compiles cleanly now.

Thanks, it does compile cleanly now. When I tried to run the executable however, it does not run correctly. From the code placement in the map file, I see that the cc32xx_debug section is now occupying 0xc, moving everything else forward:

cc32xx_debug    0x0000000001000000        0xc
                0x0000000000000000                . = 0x0
 *(SORT_BY_ALIGNMENT(.dbghdr))
 .dbghdr        0x0000000001000000        0xc zephyr/boards/arm/cc3220sf_launchxl/libboards__arm__cc3220sf_launchxl.a(dbghdr.c.obj)
                0x0000000001000000                ulDebugHeader
 *(SORT_BY_ALIGNMENT(.dbghdr.*))

text_section_offset
                0x000000000100000c      0x800
                0x0000000000000800                . = 0x800
 *fill*         0x000000000100000c      0x800 
                0x000000000100080c                . = ALIGN (0x4)

vector_table    0x000000000100080c      0x308
                0x000000000100080c                _vector_start = .
 *(SORT_BY_ALIGNMENT(.exc_vector_table))
 *(SORT_BY_ALIGNMENT(.exc_vector_table.*))
 .exc_vector_table._vector_table_section

Here is how it was before the change:

text            0x0000000001000000      0xb08
                0x0000000000000000                . = 0x0
 *(SORT_BY_ALIGNMENT(.dbghdr))
 .dbghdr        0x0000000001000000        0xc zephyr/boards/arm/cc3220sf_launchxl/libboards__arm__cc3220sf_launchxl.a(dbghdr.c.obj)
                0x0000000001000000                ulDebugHeader
 *(SORT_BY_ALIGNMENT(.dbghdr.*))
                0x0000000000000800                . = 0x800
 *fill*         0x000000000100000c      0x7f4 
                0x0000000001000800                . = ALIGN (0x4)
                0x0000000001000800                _vector_start = .
 *(SORT_BY_ALIGNMENT(.exc_vector_table))
 *(SORT_BY_ALIGNMENT(.exc_vector_table.*))
 .exc_vector_table._vector_table_section

Is it possible to preserve the original placement?

This commit does the following: - renames the 'text' ROM section to 'rom_start', to reflect that this section is the first section of the image. - renames the 'TEXT_SECTION_NAME_2' section to 'text', since that section (whose start is pointed by _image_text_start) holds the entire image text section. The commit removes the confusion by having multiple ROM sections named as 'text' in ARM Cortex-M builds. Signed-off-by: Ioannis Glaropoulos <[email protected]> Signed-off-by: Øyvind Rønningstad <[email protected]>

openocd linker sections are not supposed to be part of the vector table sections. Place the sections after we define the _vector_end linker symbol. Signed-off-by: Ioannis Glaropoulos <[email protected]>

oyvindronningstad · 2020-01-22T12:38:06Z

Is it possible to preserve the original placement?

Thanks again @vanti and sorry I missed this.

This exposes a bigger problem, namely that moving the location counter (".") to a given offset from the start of the image is broken with this. It's possible to fix by changing

. = CONFIG_TEXT_SECTION_OFFSET;

to

. = ABSOLUTE(CONFIG_TEXT_SECTION_OFFSET + _image_rom_start);

but this I feel is quite an invasive change, since every place that moves the location counter has to be changed in the same way.

I suggest we go back to Ioannis' proposal, but call the section "rom_start", so it is defined as the "first" section, which makes it safe to put input sections there if they need such an offset.

In zephyr_linker_sources(). This is done since the point of the location is to place things at given offsets. This can only be done consistenly if the linker code is placed into the _first_ section. All uses of TEXT_START are replaced with ROM_START. ROM_START is only supported in some arches, as some arches have several custom sections before text. These don't currently have ROM_START or TEXT_START available, but that could be added with a bit of refactoring in their linker script. No SORT_KEYs are changed. This also fixes an error introduced when TEXT_START was added, where TEXT_SECTION_OFFSET was applied to riscv's common linker.ld instead of to openisa_rv32m1's specific linker.ld. Signed-off-by: Øyvind Rønningstad <[email protected]>

oyvindronningstad · 2020-01-22T12:45:45Z

This is now reverted back to something close to Ioannis's initial proposal, with an added commit to rename TEXT_START to ROM_START and remove it from some arches (since they have very specific sections as their first section).

ioannisg · 2020-01-22T13:08:42Z

This is now reverted back to something close to Ioannis's initial proposal, with an added commit to rename TEXT_START to ROM_START and remove it from some arches (since they have very specific sections as their first section).

@SebastianBoe @hakonfam could you revisit this, since it has been re-done?

vanti

Thanks for the fix. It now works on SimpleLink. LGTM!

hakonfam · 2020-01-23T08:38:01Z

include/arch/arm/aarch32/cortex_m/scripts/linker.ld

@@ -151,7 +151,7 @@ SECTIONS

 #endif /* CONFIG_CODE_DATA_RELOCATION */

-    SECTION_PROLOGUE(_TEXT_SECTION_NAME_2,,)
+    SECTION_PROLOGUE(_TEXT_SECTION_NAME,,)


Should we do the same fix for cortex-r ?

Will do that on separate PR, hopefully by the cortex r contributors

hakonfam · 2020-01-23T08:40:40Z

include/arch/arm/aarch32/cortex_m/scripts/linker.ld

@@ -151,7 +151,7 @@ SECTIONS

 #endif /* CONFIG_CODE_DATA_RELOCATION */

-    SECTION_PROLOGUE(_TEXT_SECTION_NAME_2,,)
+    SECTION_PROLOGUE(_TEXT_SECTION_NAME,,)


I see that _TEXT_SECTION_NAME_2 is used in scripts/sanitycheck for doing some size calculation. Should the script be updated to reflect these changes?

Yeah, we left it there because we don't fix the cortex-r now. Will make sure I ask @stephanosio for the cortex r fix ::)

Does this break the size calculation? What consequences does this have?

@SebastianBoe I am not sure where "this" exactly refers to; i'll try to reply, anyways.

The current patch only renames the "TEXT_SECTION_NAME_2" section to "text", and the original "text" section to rom_start; note that that section contains the vector table.

The patch does not move any symbols between sections.

Where does size calculation take place?

By this I meant this patch. Will see if I can find the relevant sanitycheck code.

It appears that sanitycheck believes that _TEXT_SECTION_NAME_2 is in the rw area.

Which I assume is wrong, so this patch should not cause any regression, it might even fix
the calculation.

https://github.com/zephyrproject-rtos/zephyr/blob/master/scripts/sanitycheck#L1074

ioannisg requested review from galak and MaureenHelm as code owners January 13, 2020 23:00

ioannisg requested review from stephanosio, andrewboie and wentongwu January 13, 2020 23:00

ioannisg added area: ARM ARM (32-bit) Architecture bug The issue is a bug, or the PR is fixing a bug labels Jan 13, 2020

zephyrbot added the area: API Changes to public APIs label Jan 13, 2020

andrewboie previously approved these changes Jan 13, 2020

View reviewed changes

include/arch/arm/aarch32/cortex_m/scripts/linker.ld Show resolved Hide resolved

ioannisg added this to the v2.2.0 milestone Jan 15, 2020

ioannisg mentioned this pull request Jan 15, 2020

ARM: Core Stack Improvements/Bug fixes for 2.2 release #21201

Closed

18 tasks

andrewboie approved these changes Jan 15, 2020

View reviewed changes

wentongwu approved these changes Jan 16, 2020

View reviewed changes

ioannisg mentioned this pull request Jan 16, 2020

_TEXT_SECTION_NAME_2 on ARM Cortex-R #21650

Closed

nashif approved these changes Jan 16, 2020

View reviewed changes

ioannisg requested a review from oyvindronningstad January 17, 2020 08:46

oyvindronningstad reviewed Jan 17, 2020

View reviewed changes

ioannisg force-pushed the arch_arm_cortex_m_text_section_fix branch from 07472c6 to e5b911f Compare January 17, 2020 13:35

oyvindronningstad added the DNM This PR should not be merged (Do Not Merge) label Jan 17, 2020

ioannisg requested review from andyross and SebastianBoe as code owners January 20, 2020 11:59

vanti requested changes Jan 20, 2020

View reviewed changes

arch: arm: cortex-m: linker.ld: do not indent preprocessor directives

b5767c7

We should not use indentation for pre-processor directives. This commit fixes the indentation in the ARM Cortex-M linker script. Signed-off-by: Ioannis Glaropoulos <[email protected]>

oyvindronningstad force-pushed the arch_arm_cortex_m_text_section_fix branch from f73f67a to ec95fda Compare January 21, 2020 09:35

oyvindronningstad force-pushed the arch_arm_cortex_m_text_section_fix branch from ec95fda to 1200a1f Compare January 21, 2020 15:08

ioannisg requested review from kgugala, nategraff-sifive, pgielda, ruuddw and vonhust as code owners January 21, 2020 15:08

oyvindronningstad force-pushed the arch_arm_cortex_m_text_section_fix branch from 1200a1f to e98ca71 Compare January 21, 2020 17:37

ioannisg added 2 commits January 22, 2020 13:34

arch: arm: cortex-m: move openocd sections after _vector_end

5875f6c

openocd linker sections are not supposed to be part of the vector table sections. Place the sections after we define the _vector_end linker symbol. Signed-off-by: Ioannis Glaropoulos <[email protected]>

oyvindronningstad force-pushed the arch_arm_cortex_m_text_section_fix branch from e98ca71 to 3eb1516 Compare January 22, 2020 12:43

zephyrbot added the area: ARC ARC Architecture label Jan 22, 2020

oyvindronningstad requested review from SebastianBoe and hakonfam January 22, 2020 12:58

ioannisg requested a review from vanti January 22, 2020 13:21

vanti approved these changes Jan 22, 2020

View reviewed changes

hakonfam reviewed Jan 23, 2020

View reviewed changes

SebastianBoe approved these changes Jan 23, 2020

View reviewed changes

ioannisg requested a review from oyvindronningstad January 23, 2020 09:42

ioannisg added the area: Architectures label Jan 23, 2020

hakonfam approved these changes Jan 23, 2020

View reviewed changes

andrewboie merged commit 05f0d85 into zephyrproject-rtos:master Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arch arm cortex m text section fix #21874

Arch arm cortex m text section fix #21874

ioannisg commented Jan 13, 2020 •

edited

Loading

andrewboie left a comment

andrewboie commented Jan 13, 2020

ioannisg commented Jan 14, 2020

andrewboie commented Jan 14, 2020

ioannisg commented Jan 14, 2020

andrewboie commented Jan 14, 2020

ioannisg commented Jan 15, 2020

andrewboie commented Jan 15, 2020

oyvindronningstad left a comment

oyvindronningstad Jan 17, 2020

ioannisg Jan 17, 2020

oyvindronningstad commented Jan 17, 2020

vanti left a comment

oyvindronningstad commented Jan 21, 2020 •

edited

Loading

vanti commented Jan 21, 2020

oyvindronningstad commented Jan 22, 2020

oyvindronningstad commented Jan 22, 2020 •

edited

Loading

ioannisg commented Jan 22, 2020

vanti left a comment

hakonfam Jan 23, 2020

ioannisg Jan 23, 2020

hakonfam Jan 23, 2020

ioannisg Jan 23, 2020

SebastianBoe Jan 23, 2020

ioannisg Jan 23, 2020

SebastianBoe Jan 23, 2020

SebastianBoe Jan 23, 2020 •

edited

Loading

Arch arm cortex m text section fix #21874

Arch arm cortex m text section fix #21874

Conversation

ioannisg commented Jan 13, 2020 • edited Loading

andrewboie left a comment

Choose a reason for hiding this comment

andrewboie commented Jan 13, 2020

ioannisg commented Jan 14, 2020

andrewboie commented Jan 14, 2020

ioannisg commented Jan 14, 2020

andrewboie commented Jan 14, 2020

ioannisg commented Jan 15, 2020

andrewboie commented Jan 15, 2020

oyvindronningstad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyvindronningstad commented Jan 17, 2020

vanti left a comment

Choose a reason for hiding this comment

oyvindronningstad commented Jan 21, 2020 • edited Loading

vanti commented Jan 21, 2020

oyvindronningstad commented Jan 22, 2020

oyvindronningstad commented Jan 22, 2020 • edited Loading

ioannisg commented Jan 22, 2020

vanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

ioannisg commented Jan 13, 2020 •

edited

Loading

oyvindronningstad commented Jan 21, 2020 •

edited

Loading

oyvindronningstad commented Jan 22, 2020 •

edited

Loading

SebastianBoe Jan 23, 2020 •

edited

Loading