On Thu, Nov 24, 2022 at 12:40:44AM +0100, Pali Rohár wrote:
On Wednesday 23 November 2022 18:15:17 Tom Rini wrote:
On Wed, Nov 23, 2022 at 11:50:59PM +0100, Pali Rohár wrote:
On Tuesday 22 November 2022 12:31:56 Tom Rini wrote:
It is a bad idea, and more modern toolchains will fail, if you declare an assembly function to be global and then weak, instead of declaring it weak to start with. Update assorted assembly files to use the WEAK macro directly.
Signed-off-by: Tom Rini trini@konsulko.com
During debugging of Nokia N900 code I was looking at this and I originally thought that this redefinition is the issue why N900 u-boot did not work... (but as we now know, the n900 issue was somewhere else).
So I agree with this change, feel free to add my:
Reviewed-by: Pali Rohár pali@kernel.org
... but even after this change, linked u-boot.bin binary is not-so-correct. It works but has an issue: In final u-boot.bin binary there is both weak and non-weak version of every weak function. You can verify it for example by looking at "save_boot_params" code (really code, not just symbol) in u-boot ELF binary.
The reason for this is that linker (even LTO enabled) cannot eliminate code for weak version of function because it does not know how to "drop" it from binary/assembly code. So linker just set that non-weak version of function is active and let non-weak version present in binary as probably dead code.
This is affected only by assembly files, not by C files, because gcc is called with -ffunction-sections -fdata-sections flags which cause that every (weak) function is in its separate section (so C function "int abc() { ... }" is put into the section ".text.abc" instead of ".text") and linker's flag --gc-sections (or LTO optimization) then drop all unreferenced sections.
I do not know how fix this issue in assembly files. But cannot be WEAK macro modified to change section to ".text.<entry_name>" to mimic C compiler behavior? Would this cause any issues?
Yes, you're right about the cause, and potential solution. If you can come up with a way to get each assembly function put in to a separate .text.funcname section, that would be great and much appreciated. I think I tried this at one point a long long time ago and it did work, but I didn't have something clean, either. I think I was hoping that the linux kernel folks would solve it in time, but they decided the time/effort for --gc-sections wasn't worth it, in the end.
I quickly looked at this. If "as" is invoked with --sectname-subst flag then it is possible to use '.section %S.<func_name>' and '.previous' directives. See documentation where is example of that: https://sourceware.org/binutils/docs/as/Section.html#ELF-Version
I experimented with adding into include/linux/linkage.h:
#define WEAKSECT(name) \ .section .text.name ASM_NL \ WEAK(name)
#define ENDPROCSECT(name) \ ENDPROC(name) ASM_NL \ .previous
And then defining in arch/arm/cpu/armv7/start.S:
WEAKSECT(save_boot_params) b save_boot_params_ret ENDPROCSECT(save_boot_params)
(Note that n900 has custom non-weak save_boot_params)
Then I run:
make CROSS_COMPILE=arm-linux-gnueabi- nokia_rx51_defconfig u-boot.bin KBUILD_LDFLAGS="--print-gc-sections"
And it showed me:
ld: removing unused section '.text.save_boot_params' in file 'arch/arm/cpu/armv7/start.o'
So seems that it is working.
For proper integration it would be needed to integrate --sectname-subst flag support and then replace all usage by new macros.
That's all good to know, thanks for digging more. It would be good to know if a quick and dirty experimental patch showed enough size savings to be worth a more full pursuit or if we really don't have many / any unused assembly functions or what we do have unused could be more easily handled with an ifdef or refactoring into multiple files.