Misleading names

Misleading names

[jacob navia]

Hi
The function riscv_elf_ignore_reloc, contrary to its name, does NOT ignore the relocation!

/* Special handler for relocations which don't have to be relocated. This
 * function just simply return bfd_reloc_ok.  */
static bfd_reloc_status_type riscv_elf_ignore_reloc(bfd * abfd ATTRIBUTE_UNUSED,
                            arelent *   reloc_entry,
                    asymbol *   symbol ATTRIBUTE_UNUSED,
                        void         *data ATTRIBUTE_UNUSED,
                            asection *  input_section,
                            bfd *       output_bfd,
                    char        **error_message ATTRIBUTE_UNUSED)
{
    if (output_bfd != NULL) 
        reloc_entry->address += input_section->output_offset;
    return bfd_reloc_ok;
}

Despite its name, and the associated comment, nothing is ignored here and the relocation address is modified.

Long, descriptive names are very good WHEN THEY REFLECT THE TRUTH!
Otherwise they are completely misleading.

Of course this is not a bug. 

FILE: elfe-riscv.c 
LINE:  1053

Jacob

Re: Misleading names

[Alan Modra]

On Sun, Sep 03, 2023 at 11:59:28PM +0200, jacob navia wrote:
> Hi
> The function riscv_elf_ignore_reloc, contrary to its name, does NOT ignore the relocation!
> 
> /* Special handler for relocations which don't have to be relocated. This
>  * function just simply return bfd_reloc_ok.  */
> static bfd_reloc_status_type riscv_elf_ignore_reloc(bfd * abfd ATTRIBUTE_UNUSED,
>                             arelent *   reloc_entry,
>                     asymbol *   symbol ATTRIBUTE_UNUSED,
>                         void         *data ATTRIBUTE_UNUSED,
>                             asection *  input_section,
>                             bfd *       output_bfd,
>                     char        **error_message ATTRIBUTE_UNUSED)
> {
>     if (output_bfd != NULL) 
>         reloc_entry->address += input_section->output_offset;
>     return bfd_reloc_ok;
> }
> 
> Despite its name, and the associated comment, nothing is ignored here and the relocation address is modified.
> 
> Long, descriptive names are very good WHEN THEY REFLECT THE TRUTH!
> Otherwise they are completely misleading.
> 
> Of course this is not a bug. 

There is nothing wrong with the name.  No relocation of section
contents is done.  The only change made here is for ld -r to keep the
relocation associated with the same location in the output as it was
in the input.

That said, I think it would be better for the R_RISCV_SET_ULEB128
and R_RISCV_SUB_ULEB128 howto to use a special function something like
ppc64_elf_unhandled_reloc.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Misleading names

[jacob navia]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

Hi Alain
In the context of an assembler, not a linker, input and output bed are the same: std output.

The only call done to this special function is in bfd_install_relocation:

        cont = howto->special_function(abfd,reloc_entry,symbol,
        /* XXX - Non-portable! */
                           ((bfd_byte *) data_start
                        - data_start_offset),
                      input_section,abfd,error_message);

Stdoutput is passed to both bfd pointers expected.

As far as I see, this is a modification of the relocation, but I could be wrong.
Note that stdoutput can NEVER be NULL, so the test doesn’t do anything here .

What is surely difficult is that this functions are used in the linker context and in the assembler context, what makes making any modification very difficult. It could be better to separate a bit those 2 different functionalities, but I do not know the linker enough to be sure.

Thanks for your input.

Jacob

> Le 4 sept. 2023 à 02:04, Alan Modra <amodra@gmail.com> a écrit :
> 
> On Sun, Sep 03, 2023 at 11:59:28PM +0200, jacob navia wrote:
>> Hi
>> The function riscv_elf_ignore_reloc, contrary to its name, does NOT ignore the relocation!
>> 
>> /* Special handler for relocations which don't have to be relocated. This
>> * function just simply return bfd_reloc_ok.  */
>> static bfd_reloc_status_type riscv_elf_ignore_reloc(bfd * abfd ATTRIBUTE_UNUSED,
>>                            arelent *   reloc_entry,
>>                    asymbol *   symbol ATTRIBUTE_UNUSED,
>>                        void         *data ATTRIBUTE_UNUSED,
>>                            asection *  input_section,
>>                            bfd *       output_bfd,
>>                    char        **error_message ATTRIBUTE_UNUSED)
>> {
>>    if (output_bfd != NULL) 
>>        reloc_entry->address += input_section->output_offset;
>>    return bfd_reloc_ok;
>> }
>> 
>> Despite its name, and the associated comment, nothing is ignored here and the relocation address is modified.
>> 
>> Long, descriptive names are very good WHEN THEY REFLECT THE TRUTH!
>> Otherwise they are completely misleading.
>> 
>> Of course this is not a bug. 
> 
> There is nothing wrong with the name.  No relocation of section
> contents is done.  The only change made here is for ld -r to keep the
> relocation associated with the same location in the output as it was
> in the input.
> 
> That said, I think it would be better for the R_RISCV_SET_ULEB128
> and R_RISCV_SUB_ULEB128 howto to use a special function something like
> ppc64_elf_unhandled_reloc.
> 
> -- 
> Alan Modra
> Australia Development Lab, IBM

Re: Misleading names

[Alan Modra]

On Mon, Sep 04, 2023 at 08:29:02AM +0200, jacob navia wrote:
> Hi Alain
> In the context of an assembler, not a linker, input and output bed are the same: std output.
> 
> The only call done to this special function is in bfd_install_relocation:
> 
>         cont = howto->special_function(abfd,reloc_entry,symbol,
>         /* XXX - Non-portable! */
>                            ((bfd_byte *) data_start
>                         - data_start_offset),
>                       input_section,abfd,error_message);
> 
> Stdoutput is passed to both bfd pointers expected.
> 
> As far as I see, this is a modification of the relocation, but I could be wrong.

It isn't, because input_section->output_offset is always zero for gas.

> Note that stdoutput can NEVER be NULL, so the test doesn’t do anything here .
> 
> What is surely difficult is that this functions are used in the linker context and in the assembler context, what makes making any modification very difficult. It could be better to separate a bit those 2 different functionalities, but I do not know the linker enough to be sure.

Yes, there is a lot wrong with bfd_install_relocation, but don't
underestimate the difficulty of cleaning things up.  There are lots of
hacks, eg. see md_apply_fix comments, to work around odd things in
bfd_install_relocation.  It you change anything there, you need to
ensure *all* currently supported targets still produce working object
files.

> 
> Thanks for your input.
> 
> Jacob
> 
> > Le 4 sept. 2023 à 02:04, Alan Modra <amodra@gmail.com> a écrit :
> > 
> > On Sun, Sep 03, 2023 at 11:59:28PM +0200, jacob navia wrote:
> >> Hi
> >> The function riscv_elf_ignore_reloc, contrary to its name, does NOT ignore the relocation!
> >> 
> >> /* Special handler for relocations which don't have to be relocated. This
> >> * function just simply return bfd_reloc_ok.  */
> >> static bfd_reloc_status_type riscv_elf_ignore_reloc(bfd * abfd ATTRIBUTE_UNUSED,
> >>                            arelent *   reloc_entry,
> >>                    asymbol *   symbol ATTRIBUTE_UNUSED,
> >>                        void         *data ATTRIBUTE_UNUSED,
> >>                            asection *  input_section,
> >>                            bfd *       output_bfd,
> >>                    char        **error_message ATTRIBUTE_UNUSED)
> >> {
> >>    if (output_bfd != NULL) 
> >>        reloc_entry->address += input_section->output_offset;
> >>    return bfd_reloc_ok;
> >> }
> >> 
> >> Despite its name, and the associated comment, nothing is ignored here and the relocation address is modified.
> >> 
> >> Long, descriptive names are very good WHEN THEY REFLECT THE TRUTH!
> >> Otherwise they are completely misleading.
> >> 
> >> Of course this is not a bug. 
> > 
> > There is nothing wrong with the name.  No relocation of section
> > contents is done.  The only change made here is for ld -r to keep the
> > relocation associated with the same location in the output as it was
> > in the input.
> > 
> > That said, I think it would be better for the R_RISCV_SET_ULEB128
> > and R_RISCV_SUB_ULEB128 howto to use a special function something like
> > ppc64_elf_unhandled_reloc.
> > 
> > -- 
> > Alan Modra
> > Australia Development Lab, IBM
> 

-- 
Alan Modra
Australia Development Lab, IBM