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

[Jim Wilson]

On Wed, Sep 18, 2019 at 2:19 AM Nelson Chu <nelson.chu@sifive.com> wrote:
> This patch try to optimize the LUI and AUIPC relaxations for undefined
> weak symbols.

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.

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.

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.

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.

Otherwise this looks pretty good, I think it is the right approach.  The
UNDEFWEAK_NO_DYNAMIC_RELOC is the only thing that needs to be fixed immediately
assuming I'm right, the other issues could be fixed with follow on patches.

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.

Jim