[llvm] r213012 - [RuntimeDyld] Handle endiannes differences between the host and target while
Lang Hames
lhames at gmail.com
Tue Jul 15 08:07:46 PDT 2014
Hi Alp,
>> 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.
MemoryBuffer::getBuffer() *does* return a StringRef, but at two of the three readMachOMagic callsites in this code 'Buffer' isn't a MemoryBuffer. It's an ObjectBuffer, and ObjectBuffer::getBuffer() returns a MemoryBuffer. Obviously, right? ;)
>> 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! :-)
If anyone ever does a standup comedy routine at the dev meeting I hope this stereotype makes it in there. "What is up with backend folks and MemoryBuffer? Am I right!?"
Seriously though - why would unique_ptr be inappropriate to use with MemoryBuffer? Several of MemorBuffer's own methods return unique_ptr<MemoryBuffer>.
In the context being discussed, ObjectBuffer::getBuffer() returns a MemoryBuffer which was created via MemoryBuffer::getMemoryBuffer, which constructs a new MemoryBuffer that's essentially acting as a StringRef. That may be conceptually awful, but it definitely does need to be deleted or you'll leak memory, so std::unique_ptr seems reasonable to use there. That said: the general awfulness of this is the reason I avoided calling ObjectBuffer::getBuffer() in the first place.
As I mentioned before I'm actively working to tidy up this infrastructure. ObjectBuffer is on my hit-list, I just have some other RuntimeDyld bugs/cleanup to get to first.
Cheers,
Lang.
> On Jul 14, 2014, at 11:42 PM, Alp Toker <alp at nuanti.com> wrote:
>
>
>> 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
>
More information about the llvm-commits
mailing list