<div dir="ltr"><div class=""><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 (Magic == "\xFE\xED\xFA\xCE" || Magic == "\xCE\xFA\xED\xFE")<br>
+ return 0xFEEDFACE;<br>+ else if (Magic == "\xFE\xED\xFA\xCF" || Magic == "\xCF\xFA\xED\xFE")<br>+ return 0xFEEDFACF;<br></blockquote><br></div>> Can the content really be any one of these four strings, or would it be more appropriate to convert endianness before comparison?<br>
<div class="gmail_extra"><br></div><div class="gmail_extra">It can - here we're only at the point of verifying that what we're looking at is MachO - we don't yet know the target, and in particular we don't know whether the target endianness matches the host endianness, so there's no way to convert it prior to comparison.</div>
<div class="gmail_extra"><br></div><div><div class=""><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"> - uint32_t magic = *((const uint32_t *)Buffer->getBufferStart());<br>
+ uint32_t magic = readMachOMagic(Buffer-><u></u>getBufferStart(),<br>+ Buffer->getBufferSize());<br></blockquote><br></div>Just pass Buffer->getBuffer() (yes, it's got a silly name).<br>
</div><div><br></div><div>On its own that wouldn't be safe: Buffer->getBuffer() would allocate a new MemoryBuffer, which would then have to be deleted. I could add a unique_ptr local for it, but then it's just as unattractive as what we've got. (Side note - I dislike ObjectBuffer and friends in general, and intend to make some changes to them in the near future, hopefully to remove all references to MemoryBuffer).</div>
<div><br></div><div>I agree that this patch isn't the prettiest, but it's all about to get rolled as part of the RuntimeDyldMachO cleanup work I'm doing - I promise it'll get better soon.</div><div><br></div>
<div class="gmail_extra">Thanks for the review Alp!</div><div class="gmail_extra"><br></div><div class="gmail_extra">- Lang.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 14, 2014 at 4:47 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<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 class=""><br>
On 15/07/2014 02:19, Lang Hames wrote:<br>
<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">
Author: lhames<br>
Date: Mon Jul 14 18:19:50 2014<br>
New Revision: 213012<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213012&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=213012&view=rev</a><br>
Log:<br>
[RuntimeDyld] Handle endiannes differences between the host and target while<br>
reading MachO files magic numbers in RuntimeDyld.<br>
<br>
This is required now that we're testing cross-platform JITing (via<br>
RuntimeDyldChecker), and should fix some issues that David Fang has seen on PPC<br>
builds.<br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/<u></u>ExecutionEngine/RuntimeDyld/<u></u>RuntimeDyldMachO.cpp<br>
<br>
Modified: llvm/trunk/lib/<u></u>ExecutionEngine/RuntimeDyld/<u></u>RuntimeDyldMachO.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp?rev=213012&r1=213011&r2=213012&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>ExecutionEngine/RuntimeDyld/<u></u>RuntimeDyldMachO.cpp?rev=<u></u>213012&r1=213011&r2=213012&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/<u></u>ExecutionEngine/RuntimeDyld/<u></u>RuntimeDyldMachO.cpp (original)<br>
+++ llvm/trunk/lib/<u></u>ExecutionEngine/RuntimeDyld/<u></u>RuntimeDyldMachO.cpp Mon Jul 14 18:19:50 2014<br>
@@ -120,8 +120,21 @@ public:<br>
}<br>
};<br>
+static uint32_t readMachOMagic(const char *InputBuffer, unsigned BufferSize) {<br>
</blockquote>
<br></div>
Could you StringRefize the parameter?<div class=""><br>
<br>
<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 (BufferSize < 4)<br>
+ return 0;<br>
+ StringRef Magic(InputBuffer, 4);<br>
+ if (Magic == "\xFE\xED\xFA\xCE" || Magic == "\xCE\xFA\xED\xFE")<br>
+ return 0xFEEDFACE;<br>
+ else if (Magic == "\xFE\xED\xFA\xCF" || Magic == "\xCF\xFA\xED\xFE")<br>
+ return 0xFEEDFACF;<br>
</blockquote>
<br></div>
Can the content really be any one of these four strings, or would it be more appropriate to convert endianness before comparison?<br>
<br>
<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">
+ // else<br>
</blockquote>
<br>
Stray comment.<div class=""><br>
<br>
<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">
+ return 0;<br>
+}<br>
+<br>
ObjectImage *RuntimeDyldMachO::<u></u>createObjectImage(ObjectBuffer *Buffer) {<br>
- uint32_t magic = *((const uint32_t *)Buffer->getBufferStart());<br>
+ uint32_t magic = readMachOMagic(Buffer-><u></u>getBufferStart(),<br>
+ Buffer->getBufferSize());<br>
bool is64 = (magic == MachO::MH_MAGIC_64);<br>
assert((magic == MachO::MH_MAGIC_64 || magic == MachO::MH_MAGIC) &&<br>
"Unrecognized Macho Magic");<br>
@@ -136,7 +149,8 @@ ObjectImage *RuntimeDyldMachO::createObj<br>
MemoryBuffer *Buffer =<br>
MemoryBuffer::getMemBuffer(<u></u>ObjFile->getData(), "", false);<br>
- uint32_t magic = *((const uint32_t *)Buffer->getBufferStart());<br>
+ uint32_t magic = readMachOMagic(Buffer-><u></u>getBufferStart(),<br>
+ Buffer->getBufferSize());<br>
</blockquote>
<br></div>
Just pass Buffer->getBuffer() (yes, it's got a silly name).<div class=""><br>
<br>
<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">
bool is64 = (magic == MachO::MH_MAGIC_64);<br>
assert((magic == MachO::MH_MAGIC_64 || magic == MachO::MH_MAGIC) &&<br>
"Unrecognized Macho Magic");<br>
@@ -955,18 +969,9 @@ relocation_iterator RuntimeDyldMachO::pr<br>
bool<br>
RuntimeDyldMachO::<u></u>isCompatibleFormat(const ObjectBuffer *InputBuffer) const {<br>
- if (InputBuffer->getBufferSize() < 4)<br>
- return false;<br>
- StringRef Magic(InputBuffer-><u></u>getBufferStart(), 4);<br>
- if (Magic == "\xFE\xED\xFA\xCE")<br>
- return true;<br>
- if (Magic == "\xCE\xFA\xED\xFE")<br>
- return true;<br>
- if (Magic == "\xFE\xED\xFA\xCF")<br>
- return true;<br>
- if (Magic == "\xCF\xFA\xED\xFE")<br>
- return true;<br>
- return false;<br>
+ uint32_t Magic = readMachOMagic(InputBuffer-><u></u>getBufferStart(),<br>
+ InputBuffer->getBufferSize());<br>
</blockquote>
<br></div>
Ditto.<div class=""><div class="h5"><br>
<br>
<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">
+ return (Magic == 0xFEEDFACE || Magic == 0xFEEDFACF);<br>
}<br>
bool RuntimeDyldMachO::<u></u>isCompatibleFile(const object::ObjectFile *Obj) const {<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br></div></div><span class=""><font color="#888888">
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br></div></div>