<div dir="ltr">On 12 December 2013 12:21, Mark Seaborn <span dir="ltr"><<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">On 11 December 2013 14:05, Sasa Stankovic <span dir="ltr"><<a href="mailto:Sasa.Stankovic@rt-rk.com" target="_blank">Sasa.Stankovic@rt-rk.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+      // Check for expressions %hi(tmp) and %lo(tmp), where tmp = sym1-sym2.<br>

</blockquote></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+      if (Target.getSymA() != NULL && Target.getSymA()->getSymbol().isVariable()<br>

+          && Target.getSymB() == NULL && Target.getConstant() == 0) {<br>+        const MCExpr *SymValue = Target.getSymA()->getSymbol()<br>+                                       .getVariableValue();<br>

+        const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(SymValue);<br>+<br>+        if (BE && BE->getOpcode() == MCBinaryExpr::Sub) {<br>+          const MCSymbolRefExpr *LHS = dyn_cast<MCSymbolRefExpr>(BE->getLHS());<br>

+          const MCSymbolRefExpr *RHS = dyn_cast<MCSymbolRefExpr>(BE->getRHS());<br>+          if (LHS != NULL && LHS->getSymbol().isDefined()<br>+              && RHS != NULL && RHS->getSymbol().isDefined()) {<br>

+            // Don't emit relocation for expressions %hi(tmp) and %lo(tmp),<br>+            // where tmp = sym1-sym2.<br>+            IsResolved = true;<br>+<br>+            // Calculate symbol value.<br>+            uint64_t Value1 =<br>

+              Layout.getSymbolOffset(&Asm.getSymbolData(LHS->getSymbol()));<br>+            uint64_t Value2 =<br>+              Layout.getSymbolOffset(&Asm.getSymbolData(RHS->getSymbol()));<br>+            Value = Value1 - Value2;<br>

+          }<br>+        }<br>+      }</blockquote><div><br></div><div>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 <a href="https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp#newcode116" target="_blank">https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp#newcode116</a>).</div>
</div></div></blockquote><div><br></div><div>To expand on that, currently if I try to compile this test code:</div><div><br></div><div> __asm__(</div><div>"lui $2, %lo(label2 - label1)\n"</div><div>"label1:\n"</div>
<div>".fill 0x54321\n"</div><div>"label2:\n");</div><div><br></div><div>Then I get the error:</div><div><div>ELFRelocs might be unstable!</div><div>UNREACHABLE executed at .../sw/llvm/lib/MC/MCELFObjectTargetWriter.cpp:54!</div>
</div><div><br></div><div>which is caused by:</div><div> 1) having two relocations generated for the same address, and</div><div> 2) the lack of a "return 0" in cmpRel() in MCELFObjectTargetWriter.cpp to handle (1).</div>
<div><br></div><div>If I fix (2) by adding a "return 0", then I get:</div><div><br></div><div>$ clang symbol_diff_asm.c -emit-llvm -S -o - | llc - -o obj.o -mtriple mips -filetype=obj<br></div><div><div>$ mipsel-linux-gnu-objdump -Dr obj.o</div>
<div>...</div><div>       0:<span class="" style="white-space:pre"> </span>3c024325 <span class="" style="white-space:pre"> </span>lui<span class="" style="white-space:pre">       </span>v0,0x4325<br></div><div><span class="" style="white-space:pre">                        </span>0: R_MIPS_LO16<span class="" style="white-space:pre">    </span>.text</div>
<div><span class="" style="white-space:pre">                    </span>0: R_MIPS_LO16<span class="" style="white-space:pre">    </span>.text</div></div><div><br></div><div>We end up with two R_MIPS_LO16 relocations for the same address because getExprOpValue() in MipsMCCodeEmitter.cpp does this:</div>
<div><br></div><div><div>  if (Kind == MCExpr::Binary) {</div><div>    unsigned Res = getExprOpValue(cast<MCBinaryExpr>(Expr)->getLHS(), Fixups);</div><div>    Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups);</div>
<div>    return Res;</div><div>  }</div></div><div><br></div><div>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.</div>
<div><br></div><div>Cheers,</div><div>Mark</div><div><br></div></div></div></div>