<div dir="auto"><div>If you're refactoring this code, one request I have is to look at the calls to isPreemptible. We call it multiple times (sometimes in subroutines). IIRC when I was just staring at the code (for some unrelated reason) it wasn't obvious at a glance whether all the different calls compute the same value. Saving isPreemptible in a variable where possible would make this clearer (and when the value does need to be recomputed, then some comments would be really helpful explaining why).</div><div dir="auto"><br></div><div dir="auto">For example, adjustExpr can modify NeedsPltAddr, and isPreemptible depends on that.</div><div dir="auto"><br></div><div dir="auto">-- Sean Silva<br><div class="gmail_extra" dir="auto"><br><div class="gmail_quote">On Mar 23, 2017 7:02 PM, "Rui Ueyama via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ruiu<br>
Date: Thu Mar 23 19:50:37 2017<br>
New Revision: 298672<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=298672&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=298672&view=rev</a><br>
Log:<br>
Early continue. NFC.<br>
<br>
The original code is a big `if` and `else` which ends with `continue`<br>
like this:<br>
<br>
if (cond) {<br>
...<br>
// fall through<br>
} else {<br>
...<br>
continue;<br>
}<br>
<br>
This patch rewrites it with the following.<br>
<br>
if (!cond) {<br>
...<br>
continue;<br>
}<br>
...<br>
<br>
Modified:<br>
lld/trunk/ELF/Relocations.cpp<br>
<br>
Modified: lld/trunk/ELF/Relocations.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Relocations.cpp?rev=298672&r1=298671&r2=298672&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/lld/trunk/ELF/<wbr>Relocations.cpp?rev=298672&r1=<wbr>298671&r2=298672&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- lld/trunk/ELF/Relocations.cpp (original)<br>
+++ lld/trunk/ELF/Relocations.cpp Thu Mar 23 19:50:37 2017<br>
@@ -736,26 +736,8 @@ static void scanRelocs(InputSectionBase<br>
if (Expr == R_TLSDESC_CALL)<br>
continue;<br>
<br>
- if (needsPlt(Expr) ||<br>
- refersToGotEntry(Expr) || !isPreemptible(Body, Type)) {<br>
- // If the relocation points to something in the file, we can process it.<br>
- bool Constant =<br>
- isStaticLinkTimeConstant<ELFT><wbr>(Expr, Type, Body, C, RI.r_offset);<br>
-<br>
- // If the output being produced is position independent, the final value<br>
- // is still not known. In that case we still need some help from the<br>
- // dynamic linker. We can however do better than just copying the incoming<br>
- // relocation. We can process some of it and and just ask the dynamic<br>
- // linker to add the load address.<br>
- if (!Constant)<br>
- AddDyn({Target->RelativeRel, &C, Offset, true, &Body, Addend});<br>
-<br>
- // If the produced value is a constant, we just remember to write it<br>
- // when outputting this section. We also have to do it if the format<br>
- // uses Elf_Rel, since in that case the written value is the addend.<br>
- if (Constant || !RelTy::IsRela)<br>
- C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});<br>
- } else {<br>
+ if (!needsPlt(Expr) && !refersToGotEntry(Expr) &&<br>
+ isPreemptible(Body, Type)) {<br>
// We don't know anything about the finaly symbol. Just ask the dynamic<br>
// linker to handle the relocation for us.<br>
if (!Target->isPicRel(Type))<br>
@@ -783,6 +765,24 @@ static void scanRelocs(InputSectionBase<br>
continue;<br>
}<br>
<br>
+ // If the relocation points to something in the file, we can process it.<br>
+ bool Constant =<br>
+ isStaticLinkTimeConstant<ELFT><wbr>(Expr, Type, Body, C, RI.r_offset);<br>
+<br>
+ // If the output being produced is position independent, the final value<br>
+ // is still not known. In that case we still need some help from the<br>
+ // dynamic linker. We can however do better than just copying the incoming<br>
+ // relocation. We can process some of it and and just ask the dynamic<br>
+ // linker to add the load address.<br>
+ if (!Constant)<br>
+ AddDyn({Target->RelativeRel, &C, Offset, true, &Body, Addend});<br>
+<br>
+ // If the produced value is a constant, we just remember to write it<br>
+ // when outputting this section. We also have to do it if the format<br>
+ // uses Elf_Rel, since in that case the written value is the addend.<br>
+ if (Constant || !RelTy::IsRela)<br>
+ C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});<br>
+<br>
// At this point we are done with the relocated position. Some relocations<br>
// also require us to create a got or plt entry.<br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>