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

[Sasa Stankovic]

Hi Mark,

The modified patch is attached. I used the method
MCExpr::EvaluateAsAbsolute for computing the symbol value. This also
handles the case of two symbols in different sections (EvaluateAsAbsolute
won't compute symbol value in that case).

Mips LLVM assembler currently doesn't support subtractions of any kind
(neither between two symbols, nor between symbol and a constant). This is
why only "+" is handled in method MipsMCCodeEmitter::getExprOpValue.

Regards,
Sasa


> 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 --------------
A non-text attachment was scrubbed...
Name: hilo_sym_diff.patch
Type: text/x-patch
Size: 3722 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131213/6001628b/attachment.bin>