https://sourceware.org/bugzilla/show_bug.cgi?id=30354
--- Comment #9 from Torbjörn SVENSSON <torbjorn.svensson at foss dot st.com> ---
@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;
> }
--
You are receiving this mail because:
You are on the CC list for the bug.
