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

[Lang Hames]

>
> +  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, 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).

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> 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
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140714/3c22bbaf/attachment.html>