Re: MIPS patch to avoid lazy binding in la macros

[Richard Sandiford <rsandifo at redhat dot com>]

Hi Maciej,

I'm standing by the patch since I believe it does fix a real
assembler bug.  However, if you still think it should be
reverted after reading this message, I'll do so.

"Maciej W. Rozycki" <macro at ds2 dot pg dot gda dot pl> writes:
>  I object.
> 
>  Essentially you kill lazy binding for code as currently emitted by gcc
> entirely this way.

True.  I'm getting used to mips-rewrite, so I forgot what older gccs
(including trunk ;) do.  But more below.

> And I don't think you have to.  While you have
> incorrect CALL relocations in the object file, indeed, they will be
> converted to regular GOT references upon the final link as the function
> will not get a stub (as this is an undefined weak symbol as I understand;
> otherwise it wouldn't be zero or there would be other relocations related
> to taking the function's address).  If the function gets a stub
> regardless, there is a bug elsewhere, probably in BFD -- try to narrow it
> down then.

Yes, the problem I came across was with undefined weak symbols.
I think the same sort of thing could happen for defined weak symbols
as well.  Example:

        if (foo != dont_run_this)
          foo ();

I've attached a runnable test case, in the form of a shell script.
I tried both:

       CROSS=mipsel-linux-gnu- REG=24 ./tc
  and  CROSS=mipsel-linux-gnu- REG=25 ./tc

The ./main produced by the first command runs OK.  The second one
segfaults.

In the second case, the weak symbol (foo) has a stub in lib.so.
But this seems correct to me.  The only reloc that refers to the
symbol is an R_MIPS_CALL16.  And I can't find anything in the ABI
which says that undefined weak symbols are not allowed to have stubs.

FWIW, I tried the same experiment on irix (with -32).  I used gas to
assemble lib.s but used the irix ld and cc to do the rest.  The irix
linker also uses a stub for foo.  So again, the test case works if you
use REG=24, but if you use REG=25, you get:

15021780:./main: rld: Fatal Error: attempted access to unresolvable symbol in ./lib.so: foo

As I understand it, the idea behind the old behaviour was that
gcc generated:

        la $25,foo
        jal $31,$25

and we wanted GAS to treat "la $25,foo" as part of a call sequence.

But the question (which I think you asked then) is: why does gcc split
the call in the first place?  I see from the archives that one answer
was: to allow better scheduling.  But I don't think it does.

Quoting from trunk mips.md:

    (define_insn "call_internal2"
      [(call (mem (match_operand 0 "call_insn_operand" "ri"))
             (match_operand 1 "" "i"))
       (clobber (match_operand:SI 2 "register_operand" "=d"))]
      "TARGET_ABICALLS && !TARGET_LONG_CALLS"
      "*
    {
      register rtx target = operands[0];

      if (GET_CODE (target) == CONST_INT)
        return \"li\\t%^,%0\\n\\tjal\\t%2,%^\";
      else if (CONSTANT_ADDRESS_P (target))
        {
          if (GET_MODE (target) == SImode)
            return \"la\\t%^,%0\\n\\tjal\\t%2,%^\";
          else
            return \"dla\\t%^,%0\\n\\tjal\\t%2,%^\";
        }
      else if (REGNO (target) != PIC_FUNCTION_ADDR_REGNUM)
        return \"move\\t%^,%0\\n\\tjal\\t%2,%^\";
      else
        return \"jal\\t%2,%0\";
    }"
      [(set_attr "type"	"call")
       (set_attr "mode"	"none")
       (set_attr "length"	"8")])

So gcc is actually emitting:

        (call (mem (symbol_ref "foo")) ...)

as a fixed-form assembler string:

        la $25,foo
        jal $25,$31

There's no scheduling benefit here.  The only reason I can think of
doing it is to avoid a nop between the load and the jump when assembling
with -mips2.  If that is indeed the reason, it doesn't seem to be a very
good one.  gas inserts all sorts of other nops (even for -mips2) when
expanding PIC macros.  IMO we should fix the assembler instead.

Note that gcc's call expanders don't force the address into a register,
so SYMBOL_REF calls are the norm.  When we do have indirect calls, the
'r' constraint will allow any GPR, not just $25.

For example, try compiling:

    void foo (void) __attribute__ ((weak));
    void bar (void) { if (foo) foo (); }

I get:

        la      $2,foo
        .set    noreorder
        .set    nomacro
        beq     $2,$0,$L1
        sw      $28,24($sp)
        .set    macro
        .set    reorder

        move    $25,$2
        jal     $31,$25
$L1:

[I was wrong in my original mail when I said that gcc didn't copy
the GPR to $25.  Doh!  Serves me right for not checking.]

So in summary, gcc seems to be going out of its way not to emit
"jal foo" when that's exactly what the assembler wants to see.
I think the correct fix is to make historical[*] gccs use "jal %0"
for the CONSTANT_ADDRESS_P case.

I don't think we should hack bfd to prevent lazy binding in the test
case below.  Why should conforming code be penalised because of a
strange gccism?

[*] this has already been "fixed" on mips-rewrite branch.

>  One could argue the whole heuristics is fragile, to say at least, and I
> would wholeheartedly agree.  It should be replaced with explicit setting
> of relocation attributes using "%call_hi", "%call_lo", "%got_hi" and
> "%got_lo" (I think there should be two additional operations like "%call" 
> and "%got"[*] for the "la" and related macros themselves).  I discussed it
> at this list a few years ago and IIRC there was an agreement that is a
> good idea with the heuristics being a temporary solution until gcc gets
> improved.

Well, the code is still raw, but mips-rewrite does what you suggest,
at least for n32 and n64 PIC.  There's a patch (pending review ;) to
extend it to o32 PIC as well.

But gcc will probably need to have an "assembler macro" mode for
some time (for example, to support o32 on irix).

Richard

PS. I see I forgot to change the -xgot case.  I'll do that as an
obvious follow-up if the current patch stays.

Attachment: tc
Description: Text document