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

[rtl872x] linker: stop relying on .dynalib + .psram_text being contiguous and properly and similarly aligned within LMA and VMA, just copy them separately #2665

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

avtolstoy
Copy link
Member

Problem

Some user apps break expectations of .dynalib + .psram_text being contiguous and similarly aligned in LMA (flash) and VMA (PSRAM).

Solution

We've had a number of issues with LD over the years especially during compiler version upgrades, so instead of relying on a workaround that exists in system-part1 right now, simply copy .dynalib and .psram_text separately.

Steps to Test

Build system-part1 and the app below, both should run correctly.

You_re_Muted__inferencing.zip

Example App

N/A

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

…uous and properly and similarly aligned within LMA and VMA, just copy the separately
@avtolstoy avtolstoy added the bug label Jun 21, 2023
@avtolstoy avtolstoy added this to the 5.4.1 milestone Jun 21, 2023
Copy link
Member

@scott-brust scott-brust left a comment

Choose a reason for hiding this comment

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

Reproduced the fault with develop + provided app. User loop() function would result in red SOS
Confirmed the fix with this branch. User loop executes fine

@@ -50,6 +55,12 @@ __attribute__((section(".xip.text"))) void* module_user_pre_init() {
// Initialize .bss
_memset(&link_bss_location, 0, link_bss_size );

// Copy .dynalib
if ( (&link_dynalib_start != &link_dynalib_flash_start) && (link_dynalib_size != 0))
Copy link
Member

Choose a reason for hiding this comment

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

&link_dynalib_start and &link_dynalib_flash_start will always be different right? One is the the VMA in PSRAM and the other is the LMA from flash?
Is this comparison check just a holdover from the past, or if the sections happen to be the same VMA+LMA and no copying is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should be yes, just a precaution if they are the same (no LMA).

Copy link
Member

@XuGuohui XuGuohui left a comment

Choose a reason for hiding this comment

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

Just curious how the apps break expectations of .dynalib + .psram_text being contiguous.

@avtolstoy
Copy link
Member Author

@XuGuohui 🤷 Most likely because the libs used in the attached application have some special internal alignment requirements, this makes linker lay out things in a different way to what we normally see, causing a difference between LMA and VMA layout.

@avtolstoy avtolstoy merged commit 86bc4a6 into develop Jun 23, 2023
12 checks passed
@avtolstoy avtolstoy deleted the fix/p2-user-part-dynalib-alignment-lma-vma branch June 23, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants