[PATCH] RISC-V: Optimize lui and auipc relaxations for undefweak symbols

[Nelson Chu]

*Dear Jim,*

> The ChangeLog entries seem out of order, relax_func_t says Likewise and
comes
> after _bfd_riscv_relax_section, but doesn't have the same changes.  Also,
> you are adding a field to structs not a variable, and a parameter to
functions.

*OK I will fix the wrong comment :)*

> In _bfd_riscv_relax_section you are checking
>     h->root.type == bfd_link_hash_undefweak
> I think you may need to use
>     UNDEFWEAK_NO_DYNAMIC_RELOC (info, h)
> instead.  The issue is that when creating a shared library, an undef weak
> may not be defined at link time, but might be defined at run time, so the
> user has an option to force it to zero at link time or do a run-time check
> via -z [no]dynamic-undefined-weak.  Thus we have to check visibility and
> the user supplied option in addition to bfd_link_hash_undefweak.  This is
> stuff I have limited understanding of, so I'm not sure of the details, but
> if undefined weak can still lead to a run-time symbol lookup, then we
can't
> force the value to zero at link-time, so this optimization is wrong in
that
> case.  This should be checked with shared library builds.  I don't know if
> there are any linker tests for this.



*At first I thought that toolchain will use GOT and PLT if creating a
shared library.Therefore, there is no harm without checking the above case
I thought.But you are right, there may have some exceptions, and caution is
the parent of safety :)*

#define UNDEFWEAK_NO_DYNAMIC_RELOC(INFO, H)             \
  ((H)->root.type == bfd_link_hash_undefweak            \
   && (ELF_ST_VISIBILITY ((H)->other) != STV_DEFAULT    \
       || (INFO)->dynamic_undefined_weak == 0))






*Another problem is that, the value of `dynamic_undefined_weak` is always
be -1 forthe ld/testsuite/ld-riscv-elf/weakref32 testsuite with static
linking, and then theundefweak optimization is disabled if we use the macro
UNDEFWEAK_NO_DYNAMIC_RELOCto check.Maybe I missed something?*

> Did you test with a C target?  I see you converting instructions to use
x0,
> but I don't see you converting instructions into compressed form.  This
> should be possible in the LO12I case, since it can be a c.li X,0.  The
LO12S
> case is probably more complicated, but also less important.  Doing this
saves
> another 2 bytes per undefined weak reference.

*OK I will do the further optimization with another patches.*

>By the way, the SEC_MERGE and SEC_CODE checks should no longer be necessary
>in theory since Optimitech added the missing _bfd_merged_section_offset
call.
>We just haven't gotten around to trying to remove them yet.  I suspect the
>undefined_weak check here is unnecessary, since a undefined symbol can't be
>in a merge or code section.

> Trying to build on Ubuntu 18.04 with mainline, I get an error
> ../../binutils-gdb/bfd/elfnn-riscv.c: In function
‘_bfd_riscv_relax_section’:
> ../../binutils-gdb/bfd/elfnn-riscv.c:4148:12: error: ‘symval’ may be used
unini\
> tialized in this function [-Werror=maybe-uninitialized]
>        if (!relax_func (abfd, sec, sym_sec, info, rel, symval,
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           max_alignment, reserve_size, again,
>           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           &pcgp_relocs, undefined_weak))
>           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> I had to set symval to 0 in the undefined_weak if statement.
>
> Also, you are adding a lot of if statement checks for undefined_weak, but
> there is no harm in executing this code since the relaxation functions
don't
> use any of these variables when undefined_weak is true, so it doesn't
matter
> if we set them before calling the relaxation function.  Also, since
> undefined_weak is rare, it is probably faster to not have the checks,
because
> it is only rarely that we waste time computing stuff we won't use.  As
opposed
> to always checking a variable value that will almost always be false.

*Indeed I add too much unnecessary checking with 'undefined_weak'...*
*I should set the 'symval' and 'sym_sec' for undefined weak symbol, just
like other*
*normal cases, since we already have the flag 'undefined_weak' and the
value of*
*'symval' and 'sym_sec' are not important.*

*Therefore,  I can remove the most unnecessary checking if the 'symval' and
'sym_sec'*
*are set for the undefined weak symbols.*

else if (UNDEFWEAK_NO_DYNAMIC_RELOC (info, h)
                   && (relax_func == _bfd_riscv_relax_lui
                       || relax_func == _bfd_riscv_relax_pc))
            {
              /* For the lui and auipc relaxations, since the symbol
                 value of an undefined weak symbol is always be zero,
                 we can optimize the patterns into a single LI/MV/ADDI
                 instruction.  */
              undefined_weak = TRUE;
              symval = h->root.u.def.value;
              sym_sec = h->root.u.def.section;
            }
          else if (h->root.u.def.section->output_section == NULL
                   || (h->root.type != bfd_link_hash_defined
                       && h->root.type != bfd_link_hash_defweak))
            continue;
          else
            {
              symval = h->root.u.def.value;
              sym_sec = h->root.u.def.section;
            }


*Thank you very much!*
*I will send the modified patch ASAP :)*

*Thanks*
*Best Regards*
*Nelson*

<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
不含病毒。www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>