[PATCH,V6 06/10] bfd: linker: merge .ctf_frame sections

[Indu Bhagat]

On 8/15/22 06:02, Nick Clifton wrote:
> Hi Indu,
>
>
>> +ctf_frame_decoder_mark_func_deleted (struct ctf_frame_dec_info 
>> *cfd_info,
>> +                     unsigned int func_idx)
>> +{
>> +  BFD_ASSERT (func_idx < cfd_info->cfd_fde_count);
>> +  cfd_info->cfd_func_bfdinfo[func_idx].func_deleted_p = true;
>
> Just for the record, I am not a fan of assertions inside library 
> functions.
> I believe that libraries should let their users decide what to do when 
> there
> is a problem, rather than unilaterally calling abort.  (The better 
> approach
> in my opinion is to return an informative error message to the caller).
>
> That said there are plenty of other places in the BFD library where 
> assertions
> are used, so I am not going to complain about this.  I just wanted to 
> make
> sure that you knew of my feelings on this issue.
>
>

Noted.  I will work out something here.


>
>> +/* Try to parse .ctf_frame section SEC, which belongs to ABFD.  
>> Store the
>> +   information in the section's sec_info field on success. COOKIE
>> +   describes the relocations in SEC.  */
>> +
>> +void
>> +_bfd_elf_parse_ctf_frame (bfd *abfd, struct bfd_link_info *info,
>> +              asection *sec, struct elf_reloc_cookie *cookie)
>
> It seems to me that this function really ought to return a bool,
> indicating success or failure.
>

I will change it to return success/failure.


>
>> +  /* Read the ctf frame unwind information from abfd.  */
>> +  if (!bfd_malloc_and_get_section (abfd, sec, &ctfbuf))
>> +    goto fail_no_free;
>
> The name of this label seems rather ironic, given that ...
>
>> +fail_no_free:
>> +  _bfd_error_handler
>> +   (_("error in %pB(%pA); no .ctf_frame will be created"),
>> +    abfd, sec);
>> +success:
>> +  free (ctfbuf);
>
> ... it falls through into the free().
>

Oops! I will change this.

>
> Cheers
>   Nick
>