Created attachment 14826 [details] Demo In order to generate an implib containing the addresses required to jump from a non-secure function to a secure function, the secure function needs to use the attribute cmse_nonsecure_entry. With this attribute, a certain .gnu.sgstubs section in the ELF file is produced that contains these "secure callable functions". If the function with the attribute cmse_nonsecure_entry is calling some other function that happens to be in a compile unit that does not have any other function that is called from the main() function, then the debug information for this compile unit is GCed if the application is linked with --gc-sections. Attached is a small demonstration of what's happening. The linker script in the attached example is the default one with one addition: .gnu.sgstubs : { *(.gnu.sgstubs*) } I suppose this is an overlook and that this section should be added to the default linker script for Arm targets in order to not force the user to define there own linker script. The output when building the demonstration is this: $ make arm-none-eabi-gcc -mcpu=cortex-m33 -std=gnu11 -g3 -mcmse -ffunction-sections -fdata-sections -c -o bar.o bar.c arm-none-eabi-gcc -mcpu=cortex-m33 -std=gnu11 -g3 -mcmse -ffunction-sections -fdata-sections -c -o main.o main.c arm-none-eabi-gcc -mcpu=cortex-m33 -std=gnu11 -g3 -mcmse -ffunction-sections -fdata-sections -c -o foo.o foo.c arm-none-eabi-gcc -mcpu=cortex-m33 -T link.ld -static -Wl,--gc-sections -Wl,--print-gc-sections -Wl,--cmse-implib -Wl,--out-implib=./secure_nsclib.o -o test.elf bar.o main.o foo.o 2>&1 | grep -E 'removing unused section .* in file .[a-z]+\.o.' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.group' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_info' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_abbrev' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_aranges' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_ranges' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_macro' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_macro[wm4.0.c063f11b96416a377bbfa49196ca6eec]' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_line' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_str' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.comment' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.debug_frame' in file 'bar.o' c:\toolchain\arm-none-eabi\bin\ld.exe: removing unused section '.comment' in file 'foo.o' I've verified that the bug still exists with acdf60711d44d20608873bec0376688c9a80e281 and it's not limited to Windows although the output above was produced on Windows.
(In reply to Torbjörn SVENSSON from comment #0) Hi Torbjörn, > Attached is a small demonstration of what's happening. I may be mistaken here, but when an implib is being created, shouldn't the binary file that is being created be linked as a shared library not a fully linked executable ? This should stop the linker's garbage collection from even running, so the problem should not arise ... > The linker script in the attached example is the default one with one > addition: > .gnu.sgstubs : { *(.gnu.sgstubs*) } > I suppose this is an overlook and that this section should be added to the > default linker script for Arm targets in order to not force the user to > define there own linker script. Yes, that is an omission from the default scripts. I will fix it. Cheers Nick
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b29f2fda4f189a008f5f2017d403976c988ad63e commit b29f2fda4f189a008f5f2017d403976c988ad63e Author: Nick Clifton <nickc@redhat.com> Date: Mon Apr 17 15:48:45 2023 +0100 Add support for the .gnu.sgstubs section to the linker for ARM/ELF based targets. PR 30354 * emulparams/armelf.sh (OTHER_PLT_SECTIONS): Define in order to handle the .gnu.sgstubs section.
Hi Nick, Long time...! (In reply to Nick Clifton from comment #1) > (In reply to Torbjörn SVENSSON from comment #0) > Hi Torbjörn, > > > Attached is a small demonstration of what's happening. > > I may be mistaken here, but when an implib is being created, shouldn't the > binary file that is being created be linked as a shared library not a fully > linked executable ? This should stop the linker's garbage collection from > even running, so the problem should not arise ... While this is most likely true for most implibs, it's not the case with Cortex-M33 targets with TrustZone (CMSE). In this case, the implib is used as an interface between the secure and non-secure applications with a jump table (veneer functions) that is places in a non-secure callable area of the flash. Maybe this explains why the symbols got garbage collected? A simple misunderstanding about when the implib is used? Anyway, the compile units should be marked to be preserved in the CMSE use-case of implib. > > The linker script in the attached example is the default one with one > > addition: > > .gnu.sgstubs : { *(.gnu.sgstubs*) } > > I suppose this is an overlook and that this section should be added to the > > default linker script for Arm targets in order to not force the user to > > define there own linker script. > > Yes, that is an omission from the default scripts. I will fix it. Thanks for fixing this in master!
Created attachment 14851 [details] Proposed patch Hi Torbjörn Please could you try out this patch and let me know if it works for you ? Cheers Nick
(In reply to Nick Clifton from comment #4) > Please could you try out this patch and let me know if it works for you ? I've played a bit with this patch now and indeed the debug symbols are preserved, but the binary is also much smaller than before. Just looking at the patch, I'm a bit confused about the variable "extra_marks_added". The initial value is true, and in the function body, it's set to true in certain cases, but it's never set to false and thus, the if-statement at the end of the patch should always happen. Is this the desired behavior? I don't know if there has been any other cleanup lately in ld, but it looks like the elf size is much smaller with ld from master than 5c0b4ee406035917d0e50aa138194fab57ae6bf8 that was used in the Arm GCC 11.3.rel1 release. Is it expected that the elf size for the demo application would drop from ~203kB to ~31kB? Note that the size of text and data sections are still the same.
(In reply to Torbjörn SVENSSON from comment #5) > I don't know if there has been any other cleanup lately in ld, but it looks > like the elf size is much smaller with ld from master than > 5c0b4ee406035917d0e50aa138194fab57ae6bf8 that was used in the Arm GCC > 11.3.rel1 release. Is it expected that the elf size for the demo application > would drop from ~203kB to ~31kB? Note that the size of text and data > sections are still the same. Perhaps it's 1a26a53a0dee. You're welcome. :)
Created attachment 14860 [details] Proposed patch Hi Torbjörn > Just looking at the patch, I'm a bit confused about the variable "extra_marks_added". > The initial value is true, and in the function body, it's set to true in certain > cases, but it's never set to false and thus, the if-statement at the end of the patch > should always happen. Is this the desired behavior? Oops - snafu. Please try out this corrected version which initialises extra_marks_added to false... Cheers Nick
(In reply to Nick Clifton from comment #7) > Created attachment 14860 [details] > Proposed patch > > Oops - snafu. Please try out this corrected version which initialises > extra_marks_added to false... I can confirm that this patch works using GDB to step into the "bar" function in the demonstrator attached to this bug report. I've also checked that the dwarf information (readelf -w) contains the "bar" symbol. With this patch, I would consider this ticket resolved. I suppose that the patch will be sent to the ML for review. Prior to sending the patch, I can just say that there is a blank line added and an indentation change. I'm not sure if those changes were intended or if it slipped though when you did the test patch for me. And finally, thanks for proposing a patch for the issue!
@Nick: I've been trying to understand what you patch does and come up with a few questions. Below is the full function, with your patch applied, with my questions/comments inline. > static bool > elf32_arm_gc_mark_extra_sections (struct bfd_link_info *info, > elf_gc_mark_hook_fn gc_mark_hook) > { > bfd *sub; > Elf_Internal_Shdr **elf_shdrp; > asection *cmse_sec; > obj_attribute *out_attr; > Elf_Internal_Shdr *symtab_hdr; > unsigned i, sym_count, ext_start; > const struct elf_backend_data *bed; > struct elf_link_hash_entry **sym_hashes; > struct elf32_arm_link_hash_entry *cmse_hash; > bool again, is_v8m, first_bfd_browse = true; > bool extra_marks_added = false; > asection *isec; > > _bfd_elf_gc_mark_extra_sections (info, gc_mark_hook); > > out_attr = elf_known_obj_attributes_proc (info->output_bfd); > is_v8m = out_attr[Tag_CPU_arch].i >= TAG_CPU_ARCH_V8M_BASE > && out_attr[Tag_CPU_arch_profile].i == 'M'; > > /* Marking EH data may cause additional code sections to be marked, > requiring multiple passes. */ > again = true; > while (again) > { > again = false; > for (sub = info->input_bfds; sub != NULL; sub = sub->link.next) > { > asection *o; > > if (! is_arm_elf (sub)) > continue; > > elf_shdrp = elf_elfsections (sub); > for (o = sub->sections; o != NULL; o = o->next) > { > Elf_Internal_Shdr *hdr; > > hdr = &elf_section_data (o)->this_hdr; > if (hdr->sh_type == SHT_ARM_EXIDX > && hdr->sh_link > && hdr->sh_link < elf_numsections (sub) > && !o->gc_mark > && elf_shdrp[hdr->sh_link]->bfd_section->gc_mark) > { > again = true; > if (!_bfd_elf_gc_mark (info, o, gc_mark_hook)) > return false; > } > } > > /* Mark section holding ARMv8-M secure entry functions. We mark all > of them so no need for a second browsing. */ > if (is_v8m && first_bfd_browse) > { > bool debug_sec_need_to_be_marked = false; > > sym_hashes = elf_sym_hashes (sub); > bed = get_elf_backend_data (sub); > symtab_hdr = &elf_tdata (sub)->symtab_hdr; > sym_count = symtab_hdr->sh_size / bed->s->sizeof_sym; > ext_start = symtab_hdr->sh_info; > > /* Scan symbols. */ > for (i = ext_start; i < sym_count; i++) > { > cmse_hash = elf32_arm_hash_entry (sym_hashes[i - ext_start]); > > /* Assume it is a special symbol. If not, cmse_scan will > warn about it and user can do something about it. */ > if (startswith (cmse_hash->root.root.root.string, > CMSE_PREFIX)) > { > cmse_sec = cmse_hash->root.root.u.def.section; > if (!cmse_sec->gc_mark > && !_bfd_elf_gc_mark (info, cmse_sec, gc_mark_hook)) > return false; > /* The debug sections related to these secure entry > functions are marked on enabling below flag. */ > debug_sec_need_to_be_marked = true; > break; Is it correct to break the for-loop here? When the break statement is there, then only the first entry in the sym_hashes array that matches the CMSE_PREFIX criteria will passed on to the _bfd_elf_gc_mark function. Isn't it required to call this function for every cmse_sec value or would it be enough with just the first value? > } > } > > if (debug_sec_need_to_be_marked) > { > /* Looping over all the sections of the object file containing > Armv8-M secure entry functions and marking all the debug > sections. */ > for (isec = sub->sections; isec != NULL; isec = isec->next) > { > /* If not a debug sections, skip it. */ > if (!isec->gc_mark && (isec->flags & SEC_DEBUGGING)) > { > isec->gc_mark = 1; > extra_marks_added = true; > } > } > debug_sec_need_to_be_marked = false; Since the scope of the "debug_sec_need_to_be_marked" variable is now much smaller, I think this line should be removed. > } > } > } > > first_bfd_browse = false; > } > > /* PR 30354: If we have added extra marks then make sure that any > dependencies of the newly marked sections are also marked. */ > if (extra_marks_added) > _bfd_elf_gc_mark_extra_sections (info, gc_mark_hook); > > return true; > }
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e4fbcd83c2423221ddde99d50b432df7dda06f5f commit e4fbcd83c2423221ddde99d50b432df7dda06f5f Author: Nick Clifton <nickc@redhat.com> Date: Fri May 5 11:11:32 2023 +0100 Debug info is lost for functions only called from functions marked with cmse_nonsecure_entr PR 30354 * elf32-arm.c (elf32_arm_gc_mark_extra_sections): If any debug sections are marked then rerun the extra marking in order to pick up any dependencies.
(In reply to Torbjörn SVENSSON from comment #9) Hi Torbjörn > > if (!cmse_sec->gc_mark > > && !_bfd_elf_gc_mark (info, cmse_sec, gc_mark_hook)) > > return false; > > /* The debug sections related to these secure entry > > functions are marked on enabling below flag. */ > > debug_sec_need_to_be_marked = true; > > break; > Is it correct to break the for-loop here? > When the break statement is there, then only the first entry in the > sym_hashes array that matches the CMSE_PREFIX criteria will passed on to the > _bfd_elf_gc_mark function. Isn't it required to call this function for every > cmse_sec value or would it be enough with just the first value? You are correct. That was another mistake. I have fixed that too and checked in the patch. Cheers Nick