[llvm] r213012 - [RuntimeDyld] Handle endiannes differences between the host and target while

[Alp Toker]

On 15/07/2014 06:51, Lang Hames wrote:
>
>     +  if (Magic == "\xFE\xED\xFA\xCE" || Magic == "\xCE\xFA\xED\xFE")
>     +    return 0xFEEDFACE;
>     +  else if (Magic == "\xFE\xED\xFA\xCF" || Magic ==
>     "\xCF\xFA\xED\xFE")
>     +    return 0xFEEDFACF;
>
>
> > Can the content really be any one of these four strings, or would it 
> be more appropriate to convert endianness before comparison?
>
> 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.
>
>     -  uint32_t magic = *((const uint32_t *)Buffer->getBufferStart());
>     +  uint32_t magic = readMachOMagic(Buffer->getBufferStart(),
>     +  Buffer->getBufferSize());
>
>
> Just pass Buffer->getBuffer() (yes, it's got a silly name).
>
> On its own that wouldn't be safe: Buffer->getBuffer() would allocate a 
> new MemoryBuffer,

No, getBuffer() really does return a StringRef. Like I said, the 
function just happens to have a silly name.

> which would then have to be deleted. I could add a unique_ptr local for it

There's a lot of confusion around MemoryBuffer in the backend -- 
unique_ptr is basically inappropriate for the MemoryBuffer type so 
please don't do 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).
>
> 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.
>
> Thanks for the review Alp!
>
> - Lang.
>
> On Mon, Jul 14, 2014 at 4:47 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 15/07/2014 02:19, Lang Hames wrote:
>
>         Author: lhames
>         Date: Mon Jul 14 18:19:50 2014
>         New Revision: 213012
>
>         URL: http://llvm.org/viewvc/llvm-project?rev=213012&view=rev
>         Log:
>         [RuntimeDyld] Handle endiannes differences between the host
>         and target while
>         reading MachO files magic numbers in RuntimeDyld.
>
>         This is required now that we're testing cross-platform JITing (via
>         RuntimeDyldChecker), and should fix some issues that David
>         Fang has seen on PPC
>         builds.
>
>
>         Modified:
>            
>          llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp
>
>         Modified:
>         llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp
>         URL:
>         http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp?rev=213012&r1=213011&r2=213012&view=diff
>         ==============================================================================
>         ---
>         llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp (original)
>         +++
>         llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp Mon
>         Jul 14 18:19:50 2014
>         @@ -120,8 +120,21 @@ public:
>             }
>           };
>           +static uint32_t readMachOMagic(const char *InputBuffer,
>         unsigned BufferSize) {
>
>
>     Could you StringRefize the parameter?
>
>
>         +  if (BufferSize < 4)
>         +    return 0;
>         +  StringRef Magic(InputBuffer, 4);
>         +  if (Magic == "\xFE\xED\xFA\xCE" || Magic == "\xCE\xFA\xED\xFE")
>         +    return 0xFEEDFACE;
>         +  else if (Magic == "\xFE\xED\xFA\xCF" || Magic ==
>         "\xCF\xFA\xED\xFE")
>         +    return 0xFEEDFACF;
>
>
>     Can the content really be any one of these four strings, or would
>     it be more appropriate to convert endianness before comparison?
>
>         +  // else
>
>
>     Stray comment.
>
>
>         +  return 0;
>         +}
>         +
>           ObjectImage
>         *RuntimeDyldMachO::createObjectImage(ObjectBuffer *Buffer) {
>         -  uint32_t magic = *((const uint32_t *)Buffer->getBufferStart());
>         +  uint32_t magic = readMachOMagic(Buffer->getBufferStart(),
>         +  Buffer->getBufferSize());
>             bool is64 = (magic == MachO::MH_MAGIC_64);
>             assert((magic == MachO::MH_MAGIC_64 || magic ==
>         MachO::MH_MAGIC) &&
>                    "Unrecognized Macho Magic");
>         @@ -136,7 +149,8 @@ ObjectImage *RuntimeDyldMachO::createObj
>             MemoryBuffer *Buffer =
>                 MemoryBuffer::getMemBuffer(ObjFile->getData(), "", false);
>           -  uint32_t magic = *((const uint32_t
>         *)Buffer->getBufferStart());
>         +  uint32_t magic = readMachOMagic(Buffer->getBufferStart(),
>         +  Buffer->getBufferSize());
>
>
>     Just pass Buffer->getBuffer() (yes, it's got a silly name).
>
>
>           bool is64 = (magic == MachO::MH_MAGIC_64);
>             assert((magic == MachO::MH_MAGIC_64 || magic ==
>         MachO::MH_MAGIC) &&
>                    "Unrecognized Macho Magic");
>         @@ -955,18 +969,9 @@ relocation_iterator RuntimeDyldMachO::pr
>             bool
>           RuntimeDyldMachO::isCompatibleFormat(const ObjectBuffer
>         *InputBuffer) const {
>         -  if (InputBuffer->getBufferSize() < 4)
>         -    return false;
>         -  StringRef Magic(InputBuffer->getBufferStart(), 4);
>         -  if (Magic == "\xFE\xED\xFA\xCE")
>         -    return true;
>         -  if (Magic == "\xCE\xFA\xED\xFE")
>         -    return true;
>         -  if (Magic == "\xFE\xED\xFA\xCF")
>         -    return true;
>         -  if (Magic == "\xCF\xFA\xED\xFE")
>         -    return true;
>         -  return false;
>         +  uint32_t Magic = readMachOMagic(InputBuffer->getBufferStart(),
>         +  InputBuffer->getBufferSize());
>
>
>     Ditto.
>
>
>         +  return (Magic == 0xFEEDFACE || Magic == 0xFEEDFACF);
>           }
>             bool RuntimeDyldMachO::isCompatibleFile(const
>         object::ObjectFile *Obj) const {
>
>
>         _______________________________________________
>         llvm-commits mailing list
>         llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>         http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts