<html dir="ltr">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style>
<!--
@font-face
{font-family:Calibri}
@font-face
{font-family:Tahoma}
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif"}
a:link, span.MsoHyperlink
{color:blue;
text-decoration:underline}
a:visited, span.MsoHyperlinkFollowed
{color:purple;
text-decoration:underline}
span.EmailStyle17
{font-family:"Calibri","sans-serif";
color:#1F497D}
.MsoChpDefault
{font-size:10.0pt}
@page WordSection1
{margin:1.0in 1.0in 1.0in 1.0in}
-->
</style><style type="text/css" id="owaParaStyle"></style>
</head>
<body lang="EN-US" link="blue" vlink="purple" fpstyle="1" ocsi="0">
<div style="direction: ltr;font-family: Times New Roman;color: #000000;font-size: 12pt;">
<div>Hi Andy,</div>
<div><br>
</div>
<div>thanks for the response.</div>
<div>Relocation R_MIPS_26 in this particular case in runtime linker is used to hold information where the stub is (the relocation itself and addend are used here differently than in regular linker for the same relocation - this is offtopic, but just to say
I am aware of that if anyone notices). The original relocation does not get called/resolved later (after processRelocationRef), so the idea was to emit the relocation, so it can be resolved once the final address is known.</div>
<div><br>
</div>
<div>In resolveMIPSRelocation(), Value will not always point to Section.LoadAddress at the function entry, thus an option is to use LoadAddress, this is similar how FinalAddress is calculated for ELF::R_X86_64_PC32.</div>
<div><br>
</div>
<div>Last, I am aware I skipped the part when stub is already generated. I can add in the patch similar change, e.g. </div>
<div><br>
</div>
<div><font size="2" face="Courier New"><i>@@ -1031,8 +1033,9 @@ void RuntimeDyldELF::processRelocationRef(unsigned SectionID,</i></font></div>
<div><font size="2" face="Courier New"><i> // Look up for existing stub.</i></font></div>
<div><font size="2" face="Courier New"><i> StubMap::const_iterator i = Stubs.find(Value);</i></font></div>
<div><font size="2" face="Courier New"><i> if (i != Stubs.end()) {</i></font></div>
<div><font size="2" face="Courier New"><i>- resolveRelocation(Section, Offset,</i></font></div>
<div><font size="2" face="Courier New"><i>- (uint64_t)Section.Address + i->second, RelType, 0);</i></font></div>
<div><font size="2" face="Courier New"><i>+ RelocationEntry RE(SectionID, Offset, RelType, i->second);</i></font></div>
<div><font size="2" face="Courier New"><i>+ addRelocationForSection(RE, Value.SectionID);</i></font></div>
<div><font size="2" face="Courier New"><i>+</i></font></div>
<div><font size="2" face="Courier New"><i> DEBUG(dbgs() << " Stub function found\n");</i></font></div>
<div><font size="2" face="Courier New"><i> } else {</i></font></div>
<div><font size="2" face="Courier New"><i> // Create a new stub function.</i></font></div>
<div><br>
</div>
<div><br>
</div>
<div>The reason why this was skipped in the first place was that none of the tests triggered that path, so I just left it untouched until I can find the case in which the change would be effective and correct.</div>
<div><br>
</div>
<div>Petar</div>
<div><br>
</div>
<div style="font-family: Times New Roman; color: #000000; font-size: 16px">
<hr tabindex="-1">
<div id="divRpF80968" style="direction: ltr;"><font face="Tahoma" size="2" color="#000000"><b>From:</b> Kaylor, Andrew [andrew.kaylor@intel.com]<br>
<b>Sent:</b> Monday, November 18, 2013 10:49 PM<br>
<b>To:</b> Petar Jovanovic; llvm-commits@cs.uiuc.edu<br>
<b>Cc:</b> NAKAMURA Takumi<br>
<b>Subject:</b> RE: [PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known<br>
</font><br>
</div>
<div></div>
<div>
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D">I don’t think this patch is quite correct for two reasons.</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D">First, in the call to resolveMIPSRelocation, the Section parameter refers to the section which contains the location to which the relocation is being applied
while the Value parameter contains the LoadAddress of the symbol to which the relocation refers (in this case the section containing the stub).</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D">It looks like the RuntimeDyldELF handling of this relocation guarantees the section containing the symbol to which the relocation refers will always be the
same as the section which is the target of the relocation for the R_MIPS_26 relocation type, but in the general case (that is, across the ELF relocation handling) that isn’t true. The change you made to resolveMIPSRelocation will calculate the right value,
I think, but I don’t believe that change is even necessary. Unless I’m mistaken, the line “Value = Section.LoadAddress + Addend” will not change the value of Value, which already contained the Value input argument (which should have been the section load
address) + the Addend argument. If this is true, the new change will just introduce potentially misleading code.</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D">The second issue is more serious. In RuntimeDyldELF::processRelocationRef, where you made your other change, there were two possible places where the old
code would have attempted to apply the relocation immediately. The place where you made your change handles the case where a new stub is created. However, a few lines above that there is a place where the code found that the needed stub already existed and
it attempts to immediately apply a relocation referencing that stub location. I believe you also need to update that code.</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D">-Andy</span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri","sans-serif"; color:#1F497D"> </span></p>
<div>
<div style="border:none; border-top:solid #B5C4DF 1.0pt; padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt; font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt; font-family:"Tahoma","sans-serif""> Petar Jovanovic [mailto:Petar.Jovanovic@imgtec.com]
<br>
<b>Sent:</b> Monday, November 18, 2013 12:46 PM<br>
<b>To:</b> llvm-commits@cs.uiuc.edu<br>
<b>Cc:</b> Kaylor, Andrew; NAKAMURA Takumi<br>
<b>Subject:</b> [PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known</span></p>
</div>
</div>
<p class="MsoNormal"> </p>
<div>
<div>
<p class="MsoNormal"><span style="color:black">Hi everyone,</span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:black"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:black">here is a patch for several MIPS MCJIT failures.</span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:black"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:black">Regards,</span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:black">Petar</span></p>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>