%hi($tmp) and %lo($tmp) relocations for Mips backend, where $tmp = sym1 - sym2

[Mark Seaborn]

On 12 December 2013 12:21, Mark Seaborn <mseaborn at chromium.org> wrote:

> On 11 December 2013 14:05, Sasa Stankovic <Sasa.Stankovic at rt-rk.com>wrote:
>
>> +      // Check for expressions %hi(tmp) and %lo(tmp), where tmp =
>> sym1-sym2.
>>
> +      if (Target.getSymA() != NULL &&
>> Target.getSymA()->getSymbol().isVariable()
>> +          && Target.getSymB() == NULL && Target.getConstant() == 0) {
>> +        const MCExpr *SymValue = Target.getSymA()->getSymbol()
>> +                                       .getVariableValue();
>> +        const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(SymValue);
>> +
>> +        if (BE && BE->getOpcode() == MCBinaryExpr::Sub) {
>> +          const MCSymbolRefExpr *LHS =
>> dyn_cast<MCSymbolRefExpr>(BE->getLHS());
>> +          const MCSymbolRefExpr *RHS =
>> dyn_cast<MCSymbolRefExpr>(BE->getRHS());
>> +          if (LHS != NULL && LHS->getSymbol().isDefined()
>> +              && RHS != NULL && RHS->getSymbol().isDefined()) {
>> +            // Don't emit relocation for expressions %hi(tmp) and
>> %lo(tmp),
>> +            // where tmp = sym1-sym2.
>> +            IsResolved = true;
>> +
>> +            // Calculate symbol value.
>> +            uint64_t Value1 =
>> +
>>  Layout.getSymbolOffset(&Asm.getSymbolData(LHS->getSymbol()));
>> +            uint64_t Value2 =
>> +
>>  Layout.getSymbolOffset(&Asm.getSymbolData(RHS->getSymbol()));
>> +            Value = Value1 - Value2;
>> +          }
>> +        }
>> +      }
>
>
> I think you shouldn't need to add code for evaluating a Sub expression to
> a MIPS-specific file (as I commented on an earlier version of this change
> at
> https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp#newcode116
> ).
>

To expand on that, currently if I try to compile this test code:

 __asm__(
"lui $2, %lo(label2 - label1)\n"
"label1:\n"
".fill 0x54321\n"
"label2:\n");

Then I get the error:
ELFRelocs might be unstable!
UNREACHABLE executed at .../sw/llvm/lib/MC/MCELFObjectTargetWriter.cpp:54!

which is caused by:
 1) having two relocations generated for the same address, and
 2) the lack of a "return 0" in cmpRel() in MCELFObjectTargetWriter.cpp to
handle (1).

If I fix (2) by adding a "return 0", then I get:

$ clang symbol_diff_asm.c -emit-llvm -S -o - | llc - -o obj.o -mtriple mips
-filetype=obj
$ mipsel-linux-gnu-objdump -Dr obj.o
...
       0: 3c024325 lui v0,0x4325
0: R_MIPS_LO16 .text
0: R_MIPS_LO16 .text

We end up with two R_MIPS_LO16 relocations for the same address
because getExprOpValue() in MipsMCCodeEmitter.cpp does this:

  if (Kind == MCExpr::Binary) {
    unsigned Res = getExprOpValue(cast<MCBinaryExpr>(Expr)->getLHS(),
Fixups);
    Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups);
    return Res;
  }

That code is clearly wrong because it treats any binary operator, including
"-", as a "+"!  If you're going to fix "-" to work, it might be a good idea
to fix that incorrect code.

Cheers,
Mark
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131212/9ec12087/attachment.html>