Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
triplef opened this issue Mar 23, 2024 · 10 comments · Fixed by #90151
Closed

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

triplef opened this issue Mar 23, 2024 · 10 comments · Fixed by #90151
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. platform:windows release:backport

Comments

@triplef
Copy link
Member

triplef commented Mar 23, 2024

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():

  • 0x000002b224a0bdb0 before reallocateUnaligned()
  • 0x000002b224a0bdc0 in 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
Copy link
Member Author

triplef commented Mar 23, 2024

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
Copy link
Contributor

omjavaid commented Mar 26, 2024

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
Copy link
Member

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

@compnerd
Copy link
Member

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

@omjavaid
Copy link
Contributor

omjavaid commented Mar 27, 2024

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

@omjavaid omjavaid added clang:codegen IR generation bugs: mangling, exceptions, etc. platform:windows labels Mar 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 27, 2024

@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.
QList&lt;QString&gt; 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

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 triplef changed the title Crash in QList.append() on Windows ARM64 ABI mismatch between MSVC and Clang when returning std::pair on Windows ARM64 Apr 8, 2024
@triplef
Copy link
Member Author

triplef commented Apr 13, 2024

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
Copy link
Collaborator

Reduced:

struct pair {
    template <class _Other1 = int*>
    pair(int* _Val1) : first(_Val1) { }
    int* first;
};
pair returnPair() {
    return pair((int*)0);
}

efriedma-quic added a commit to efriedma-quic/llvm-project that referenced this issue Apr 26, 2024
In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes llvm#86384
efriedma-quic added a commit that referenced this issue Apr 29, 2024
…90151)

In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes #86384
@efriedma-quic efriedma-quic reopened this Apr 30, 2024
@efriedma-quic efriedma-quic added this to the LLVM 18.X Release milestone Apr 30, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Apr 30, 2024
@efriedma-quic
Copy link
Collaborator

/cherry-pick 3ab4ae9

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Apr 30, 2024
…lvm#90151)

In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes llvm#86384

(cherry picked from commit 3ab4ae9)
@llvmbot llvmbot closed this as completed Apr 30, 2024
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in LLVM Release Status Apr 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 30, 2024

/pull-request #90639

tstellar pushed a commit to llvmbot/llvm-project that referenced this issue May 1, 2024
…lvm#90151)

In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes llvm#86384

(cherry picked from commit 3ab4ae9)
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Sep 9, 2024
…lvm#90151)

In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes llvm#86384

(cherry picked from commit 3ab4ae9)
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Oct 10, 2024
…lvm#90151)

In the context of determining whether a class counts as an "aggregate",
a constructor template counts as a user-provided constructor.

Fixes llvm#86384

(cherry picked from commit 3ab4ae9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. platform:windows release:backport
Projects
Development

Successfully merging a pull request may close this issue.

7 participants