llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.4k stars 12.15k forks source link

ABI mismatch between MSVC and Clang when returning `std::pair` on Windows ARM64 #86384

Closed triplef closed 7 months ago

triplef commented 8 months ago

Using Clang 18.1.2 WoA on Windows ARM64 results in a failed assertion when adding more than 2 items to a QList due to the assertion Q_ASSERT(!data || !data->isShared()) failing in QArrayData::reallocateUnaligned. We reproduced the issue with both Clang 17 and 18 and Qt 6.5, 6.6, and 6.7.

QList<QString> list;
list.append("ONE");
list.append("TWO");
list.append("THREE"); // CRASH

In this debug session the value of the data pointer changes by 0x10 when calling ArrayData::reallocateUnaligned():

This pointer misalignment causes an invalid value for ref_, causing isShared() to return true.

Before: debug-1

After: debug-2

We submitted the bug to Qt and got the following feedback:

QTypedArrayData is-a QArrayData so the compiler is required to adjust the pointer to the base sub-object when casting. However, in this case, we expect and require that adjustment to be zero. QTypedArrayData adds no other members, so it has exactly the same size and alignment requirements as QArrayData. In fact, we actually static_assert the size equality in one of the two screenshots. I actually don't think the issue is the adjustment of the base sub-object. This appears to be a codegen issue in selecting registers. Look at dataPointer at 0x18, which is not a valid pointer, but is sizeof(QString), the next parameter. objectSize is listed as 3, which is not a valid size for QString, but is a valid flag combination. The 0x10 difference between caller and callee is explained by the fact that it is also the difference between data and dataPointer.

triplef commented 8 months ago

We are linking against official Qt releases built with MSVC.

Further feedback from Qt:

The most likely reason for the off-by-one difference is the return type of the called function:

    [[nodiscard]] static Q_CORE_EXPORT std::pair<QArrayData *, void *> reallocateUnaligned(QArrayData *data, void *dataPointer,
            qsizetype objectSize, qsizetype newCapacity, AllocationOption option) noexcept;

The return type of std::pair of two pointers is probably the reason. The caller is likely using registers r0 to r4 for the formal parameters and expects the callee to return the two pointers in r0 and t1 pair. The callee on the other hand expects the return slot to be passed as an implicit parameter in r0, with the formal parameters in r1 to r5.

omjavaid commented 8 months ago

We are linking against official Qt releases built with MSVC.

Further feedback from Qt:

The most likely reason for the off-by-one difference is the return type of the called function:

    [[nodiscard]] static Q_CORE_EXPORT std::pair<QArrayData *, void *> reallocateUnaligned(QArrayData *data, void *dataPointer,
            qsizetype objectSize, qsizetype newCapacity, AllocationOption option) noexcept;

The return type of std::pair of two pointers is probably the reason. The caller is likely using registers r0 to r4 for the formal parameters and expects the callee to return the two pointers in r0 and t1 pair. The callee on the other hand expects the return slot to be passed as an implicit parameter in r0, with the formal parameters in r1 to r5.

@triplef I have looked into this and can confirm it as a ABI mismatch between MSVC and Clang. This could be a Clang/LLVM bug because it promises to be ABI compatible with MSVC. But there are some exception to this compatibility. I am not aware of those exceptions so lets ask some experts.

@mstorsjo @compnerd do you know if this is a deliberately left incompatibility or this is a bug?

mstorsjo commented 8 months ago

I would definitely consider this a bug - I don't think we'd leave such things incompatible deliberately.

compnerd commented 8 months ago

If you are using aarch64-unknown-windows-msvc for the triple, this is definitely a bug - the ABI should be compatible with MSVC.

omjavaid commented 8 months ago

I tested following c++ code:

#include <iostream>
#include <utility>

std::pair<int*, double*> returnPair() {
    int x = 42;
    double y = 4614253070214989087.0;
    return std::make_pair(&x, &y);
}

int main() {
    std::pair<int*, double*> pair = returnPair();
    std::cout << *pair.first << " " << *pair.second << std::endl;
    return 0;
}

Function returnPair epilogue generated by both compiler is given below:

ASM generated by MS cl Compiler Version 19.37.32824 for ARM64.

    bl          |??$make_pair@HN@std@@YA?AU?$pair@HN@0@$$QEAH$$QEAN@Z|
    ldr         x0,[sp,#0x18]
    ldp         fp,lr,[sp],#0x30
    ret

ASM generated by clang-cl 17.0.6 WoA64 upstream release build.

    bl      __security_check_cookie
    ldr     x0, [sp, #8]                    // 8-byte Folded Reload
    ldr     x1, [sp, #16]                   // 8-byte Folded Reload
    .seh_startepilogue
    ldr     x30, [sp, #64]                  // 8-byte Folded Reload
    .seh_save_reg   x30, 64
    add     sp, sp, #80
    .seh_stackalloc 80
    .seh_endepilogue
    ret

MS cl is setting up return value address in x0 while clang-cl is setting up pair of values in x0 and x1

llvmbot commented 8 months ago

@llvm/issue-subscribers-clang-codegen

Author: Frederik Seiffert (triplef)

Using Clang 18.1.2 WoA on Windows ARM64 results in a failed assertion when adding more than 2 items to a QList due to the assertion [Q_ASSERT(!data || !data->isShared())](https://github.com/qt/qtbase/blob/98602c26fc97eb41e3dd7548194ca637420a31b9/src/corelib/tools/qarraydata.cpp#L229) failing in QArrayData::reallocateUnaligned. We reproduced the issue with both Clang 17 and 18 and Qt 6.5, 6.6, and 6.7. ```c++ QList<QString> list; list.append("ONE"); list.append("TWO"); list.append("THREE"); // CRASH ``` In this debug session the value of the data pointer changes by `0x10` when calling ArrayData::reallocateUnaligned(): - `0x000002b224a0bdb0` before `reallocateUnaligned()` - `0x000002b224a0bdc0` in `reallocateUnaligned()` This pointer misalignment causes an invalid value for `ref_`, causing `isShared()` to return true. Before: ![debug-1](https://github.com/llvm/llvm-project/assets/6037/9e3d2553-ef8f-4d7a-9904-0d5c0426acda) After: ![debug-2](https://github.com/llvm/llvm-project/assets/6037/386c57a4-d560-43c3-95d8-7ccc4469a8de) We [submitted](https://bugreports.qt.io/browse/QTBUG-123617) the bug to Qt and got the following feedback: > `QTypedArrayData` is-a `QArrayData` so the compiler is required to adjust the pointer to the base sub-object when casting. However, in this case, we expect and require that adjustment to be zero. `QTypedArrayData` adds no other members, so it has exactly the same size and alignment requirements as `QArrayData`. In fact, we actually static_assert the size equality in one of the two screenshots. > I actually don't think the issue is the adjustment of the base sub-object. This appears to be a codegen issue in selecting registers. Look at dataPointer at `0x18`, which is not a valid pointer, but is `sizeof(QString)`, the next parameter. objectSize is listed as 3, which is not a valid size for `QString`, but is a valid flag combination. The 0x10 difference between caller and callee is explained by the fact that it is also the difference between data and dataPointer.
triplef commented 7 months ago

This I think somewhat related issue #88273 points to https://reviews.llvm.org/D60348 implementing struct returns on ARM64.

ARM64 ABI docs about return values: https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions#return-values

efriedma-quic commented 7 months ago

Reduced:

struct pair {
    template <class _Other1 = int*>
    pair(int* _Val1) : first(_Val1) { }
    int* first;
};
pair returnPair() {
    return pair((int*)0);
}
efriedma-quic commented 7 months ago

/cherry-pick 3ab4ae9e58c09dfd8203547ba8916f3458a0a481

llvmbot commented 7 months ago

/pull-request llvm/llvm-project#90639