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

arch: arm: cortex_r: linker.ld: Fix ROM section names #22672

Merged

Conversation

stephanosio
Copy link
Member

@stephanosio stephanosio commented Feb 10, 2020

This commit fixes the improper naming of the ROM sections.

  1. Rename the first ROM section, which was previously named using the
    _TEXT_SECTION_NAME definition, to rom_start, as this section does
    not actually represent the text section.

  2. Rename the second ROM section, which was previously named
    _TEXT_SECTION_NAME_2 which supposedly refers to the definition of
    the same name that does not exist, to _TEXT_SECTION_NAME. Note that
    this is indeed the section that contains the text section from the
    source image.

Signed-off-by: Stephanos Ioannidis [email protected]

Addresses #21650 for Cortex-R.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Second patch looks good.
The clean up stuff in the first patch is also fine, but I 'd like to see zero indentation for preprocessor directives, as this seems to be the rule (rather than the exception) in Zephyr. We did the same for cortex-m linker script :)

@ioannisg ioannisg self-assigned this Feb 10, 2020
@ioannisg ioannisg added this to the v2.2.0 milestone Feb 10, 2020
@ioannisg ioannisg added the bug The issue is a bug, or the PR is fixing a bug label Feb 10, 2020
@stephanosio
Copy link
Member Author

The clean up stuff in the first patch is also fine, but I 'd like to see zero indentation for preprocessor directives, as this seems to be the rule (rather than the exception) in Zephyr.

@ioannisg While I also tend to follow the "zero indentation for preprocessor directives" rule most of the time, in this case, the significantly improved readability from proper indentation was simply too good to pass.

Given that the indented portion is not part of any "code," this does not create the infamous readability issue involving preprocessor directives identification among the "code."

Is there a formal guideline/rule about this?

@stephanosio
Copy link
Member Author

To elaborate, I would take the former over the latter any day despite the former generally being something to avoid (in some cases with a good reason; in other cases, not so much).

#if defined(CONFIG_XIP)
  #if defined(CONFIG_IS_BOOTLOADER)
    #define RAM_SIZE (CONFIG_BOOTLOADER_SRAM_SIZE * 1K)
    #define RAM_ADDR (CONFIG_SRAM_BASE_ADDRESS + \
                     (CONFIG_SRAM_SIZE * 1K - RAM_SIZE))
  #else
    #define RAM_SIZE (CONFIG_SRAM_SIZE * 1K)
    #define RAM_ADDR CONFIG_SRAM_BASE_ADDRESS
  #endif
#else
  #define RAM_SIZE (CONFIG_SRAM_SIZE * 1K - CONFIG_BOOTLOADER_SRAM_SIZE * 1K)
  #define RAM_ADDR CONFIG_SRAM_BASE_ADDRESS
#endif
#if defined(CONFIG_XIP)
#if defined(CONFIG_IS_BOOTLOADER)
#define RAM_SIZE (CONFIG_BOOTLOADER_SRAM_SIZE * 1K)
#define RAM_ADDR (CONFIG_SRAM_BASE_ADDRESS + \
                 (CONFIG_SRAM_SIZE * 1K - RAM_SIZE))
#else
#define RAM_SIZE (CONFIG_SRAM_SIZE * 1K)
#define RAM_ADDR CONFIG_SRAM_BASE_ADDRESS
#endif
#else
#define RAM_SIZE (CONFIG_SRAM_SIZE * 1K - CONFIG_BOOTLOADER_SRAM_SIZE * 1K)
#define RAM_ADDR CONFIG_SRAM_BASE_ADDRESS
#endif

@ioannisg
Copy link
Member

@stephanosio the situation is taht 95% of Zephyr code does not use indentation in pre-processor directives. Although there is no formal requirement in the coding style guidelines, I am in favor of consistency here. It's easier to change the remaining 5% to look similar to the rest of the code :)

This commit cleans up the linker.ld file for the Cortex-R arch.

* Convert all TAB characters to SPACE.
* Fix insane placement of curly brackets.
* Fix overall text alignments.
* Remove the special handlings for the Cortex-M devices that were
  copied from `include/arm/aarch32/cortex_m/scripts/linker.ld`.

Signed-off-by: Stephanos Ioannidis <[email protected]>
This commit fixes the improper naming of the ROM sections.

1. Rename the first ROM section, which was previously named using the
  `_TEXT_SECTION_NAME` definition, to `rom_start`, as this section does
  not actually represent the text section.

2. Rename the second ROM section, which was previously named
  `_TEXT_SECTION_NAME_2` which supposedly refers to the definition of
  the same name that does not exist, to `_TEXT_SECTION_NAME`. Note that
  this is indeed the section that contains the text section from the
  source image.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@ioannisg ioannisg force-pushed the cortex_r_fix_rom_section_names branch from 6d3b691 to b5d248a Compare February 14, 2020 15:12
@ioannisg ioannisg merged commit 0e3bc28 into zephyrproject-rtos:master Feb 14, 2020
@stephanosio stephanosio deleted the cortex_r_fix_rom_section_names branch April 23, 2020 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Linker Scripts bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants