[lld] r264115 - Avoid memcpy from nullptr. NFC.

[David Blaikie via llvm-commits]

On Tue, Mar 22, 2016 at 4:16 PM, Pete Cooper <peter_cooper at apple.com> wrote:

>
> On Mar 22, 2016, at 4:14 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Mar 22, 2016 at 3:59 PM, Pete Cooper via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: pete
>> Date: Tue Mar 22 17:59:35 2016
>> New Revision: 264115
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=264115&view=rev
>> Log:
>> Avoid memcpy from nullptr.  NFC.
>>
>> This was caught by the UBSan bot.  When the atom has no size, we would
>> issue a memcpy with size0 and a nullptr for the source.
>>
>> Also, this code should never have references inside an empty atom so
>> add an assert for that while we're here.
>>
>> Modified:
>>     lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp
>>
>> Modified: lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp?rev=264115&r1=264114&r2=264115&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp (original)
>> +++ lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp Tue Mar 22
>> 17:59:35 2016
>> @@ -420,6 +420,11 @@ void ArchHandler_x86::generateAtomConten
>>                                            FindAddressForAtom
>> findSectionAddress,
>>                                            uint64_t imageBaseAddress,
>>                                            uint8_t *atomContentBuffer) {
>> +  if (!atom.size()) {
>> +    assert(atom.begin() == atom.end() &&
>> +           "Cannot have references without content");
>>
>
> Um, that seems like a problematic API, if size can be distinct from end()
> - begin() ? Perhaps some of these should be renamed to avoid confusion?
>
> Yeah…
>
> Its not very standard.  begin and end walk the Reference’s, but size() is
> about the number of bytes of content in the atom.  The two are completely
> unrelated.
>
> We should probably make size count the references, or remove size
> entirely, and have a contentSize() method where we have size() now.
>

Yeah, if the object is logically its content, then size() there might make
sense. But the fact that two different sequences were both chosen to "be"
the atom, that seems like neither's unambiguously obvious.

The begin/end could be moved into LLVM's pretty common prefix approach:
"references_begin(), references_end()" style (and a range accessor of
"references()" (if you can get away without begin/end and just having the
range accessor, all the better))


>
> Thanks for pointing this out. I became somewhat used to it, but its
> actually quite non-intuitive so i should really fix it.
>
> Cheers,
> Pete
>
>
>
>> +    return;
>> +  }
>>    // Copy raw bytes.
>>    memcpy(atomContentBuffer, atom.rawContent().data(), atom.size());
>>    // Apply fix-ups.
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160322/5d1c521b/attachment.html>