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

Ref. #926. Fixes for LTO build. #928

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ser-plu
Copy link

@ser-plu ser-plu commented Nov 8, 2022

Resolves issues, appearing during LTO build:

  • _sbrk_r is optimized out and cannot be linked afterwards
  • delay_ns_per_loop and delay_fcpu_MHz are optimized out
  • linking to static libraries requires additional script for AR tool

@salkinium
Copy link
Member

Thanks! I'll wait another day, maybe something else pops up during more testing.

@@ -33,7 +33,7 @@ typedef void (* const FunctionPointer)(void);
extern uint32_t __main_stack_top[];

// Define the vector table
modm_section(".vector_rom")
modm_section(".vector_rom") modm_used
Copy link
Member

Choose a reason for hiding this comment

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

probably also necessary for vectorsRam just below.

Basically everything that's implicitly linked needs to be marked as used? Seems like a lot of work…

Copy link
Member

@salkinium salkinium Nov 10, 2022

Choose a reason for hiding this comment

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

Can you perhaps extract the symbols of the resulting ELF files with/without LTO and diff them? That would perhaps give you a more definitive list of lost symbols.
It's scons symbols but we seem to not have it for CMake.
Otherwise try: arm-none-eabi-nm -S -C --size-sort -td file.elf

Copy link
Author

Choose a reason for hiding this comment

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

I don't see much difference any more. vectorsRam indeed is required to be used, I just use the ROM-mapped vector table in my project.

One more symbol missing is modm_abandon, but I think it is just optimised out as an empty function.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe, Undefined_Handler also requires the attribute. But I am not 100% sure

Copy link
Member

Choose a reason for hiding this comment

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

I'll have a look myself tomorrow to check if we can enable LTO by default at least for the release build.

@@ -19,7 +19,7 @@ void __modm_initialize_memory(void)
}

// ----------------------------------------------------------------------------
modm_weak modm_section("{{ no_heap_section }}")
modm_weak modm_used modm_section("{{ no_heap_section }}")
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that always trigger the warning of the heap module not being included even if heap is not used in the application code?

Copy link
Author

Choose a reason for hiding this comment

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

When heap is used, no_heap.cpp is excluded from build, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but if heap is not used, then this weak _sbrk_r acts as a "canary function" that the linker tries to link if someone tries to use malloc (which internally calls _sbrk_r via Newlib.
This function is normally garbage collected by the linker because nobody calls it, but is then DISCARDed in the linkerscript, thus the linker will fail with an error message.

@ser-plu
Copy link
Author

ser-plu commented Nov 18, 2022

FYI some numbers of LTO erformance.

I started working on LTO enabling while working on a bootloader application. At first it saved me ~400 bytes of Flash, and it was nice.
At some point I disabled LTO to have easier debug. In the end I reenabled it and got an unconvincing result: The compiler decided to use heap in one place (of course, there was no explicit dynamic memory allocation) and binded the whole std library into the binary, and the binary size exploded from 5 to 59 kB!
A couple of quick fixes have not solved the issue.

I commented out the function and compared the flash size with and without LTO. The LTO one had ~20% more, so I decided not to use it in the end (I guess, I should have gained some performance boost, but that is not really important for the bootloader).

For the bigger project I get 92,5 kB without LTO and 97 kB with LTO. Have not measured the performance yet.

So, after all I would not recommend to have LTO enabled by default in the Release build. Maybe, would be nice to have it as an additional target (Debug - Release - ReleaseLto)

@salkinium
Copy link
Member

the binary size exploded from 5 to 59 kB

Uff, that's not great. That must have pulled a lot of code in there.

I found what I was thinking of: LTO with custom linkerscripts is still not fully supported and does weird things.
https://twitter.com/modm_embedded/status/1366870337820983296
That's from 2020 though, but I haven't been able to find any progress of this work since then.

I wish there was an open-source linker specially for embedded. There are already commercial linkers, but they are expensive.

@ser-plu
Copy link
Author

ser-plu commented Nov 19, 2022 via email

@salkinium salkinium removed this from the 2022q4 milestone Jan 12, 2023
@rleh rleh added the stale ♾ label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants