[Webkit-unassigned] [Bug 226557] -Warray-bounds warning in Packed.h

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=226557

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com,
                   |                            |mcatanzaro at gnome.org

--- Comment #2 from Michael Catanzaro <mcatanzaro at gnome.org> ---
OK, at first I thought this was yet another false-positive, but now I see why GCC is complaining. Here is PackedAlignedPtr::get in Packed.h:

    T* get() const
    {
        // FIXME: PackedPtr<> can load memory with one mov by checking page boundary.
        // https://bugs.webkit.org/show_bug.cgi?id=197754
        uintptr_t value = 0;
#if CPU(LITTLE_ENDIAN)
        memcpy(&value, m_storage.data(), storageSize);
#else
        memcpy(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize), m_storage.data(), storageSize);
#endif
        if (isAlignmentShiftProfitable)
            value <<= alignmentShiftSize;

#if CPU(X86_64) && !(OS(DARWIN) || OS(LINUX) || OS(WINDOWS))
        // The AMD specification requires that the most significant 16
        // bits of any virtual address, bits 48 through 63, must be
        // copies of bit 47 (in a manner akin to sign extension).
        //
        // The above-named OSes will never allocate user space addresses
        // with bit 47 set, thus are already in canonical form.
        //
        // Reference: https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details
        constexpr unsigned shiftBits = countOfBits<uintptr_t> - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH);
        value = (bitwise_cast<intptr_t>(value) << shiftBits) >> shiftBits;
#endif

        return bitwise_cast<T*>(value);
    }

The only lines that matter are here:

        uintptr_t value = 0;
#if CPU(LITTLE_ENDIAN)
        memcpy(&value, m_storage.data(), storageSize);

Here storageSize is 5, so we copy five bytes of data from m_storage.data() to &value, which the compiler expects to be an array but is actually a uintptr_t declared on the stack. So I guess the compiler is reasonable to treat one uintptr_t as a zero-length memory region and emit the warning. We know this ought to be OK for 64-bit systems because we know that uintptr_t will be 8-bytes long, so it's not really zero-length. It is clearly intentional, so the warning should be suppressed manually.

The safety depends on (a) storageSize <= sizeof(uintptr_t) -- so maybe we should add a static_assert for that --, and (b) compiler not deciding the memcpy is undefined behavior that can be "optimized" away.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210614/2845412b/attachment.htm>