-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Handling C++ exceptions on Windows x64 corrupts local variables #42930
Comments
I think your analysis is probably correct, although I haven't checked it. I think it's reason enough to revert r367088 from the 9.0 branch and merge r370005. |
I built the 9.x release candidate and ran this test case, but I could not reproduce the problem. However, I chery picked the WinEH XMM CSR fix to the release/9.x branch: I confirmed that fixed the other similar bug (issue 43710). You can build the 9.0.1 candidate locally if you like and confirm if the issue has been fixed, but I don't plan to do any more investigation beyond this. I suspect the bug will be fixed in 9.0.1. |
Has this been fixed now in the release/9.x branch? Can we close this bug? |
While this change does appear fix the original problem I reported, I now have an access violation in different location in my product code. At this stage I don't have a lots of details but I see a "read access violation, pFunc was nullptr" that occurs in I've tried using clang built from the parent of the of this commit (2d75b24) and the "pFunc was nullptr" access violation does not occur. This leads me to think the XMM CSR fix addresses one problem but causes another. Whether this is a new bug or a related one (or indeed a bug in our code that has just been exposed), I'll let you advise whether I should raise another issue. I'd prefer to keep this bug open for a few days longer to allow me to try and come up with some more details. Also some further info.
|
I think I've located the cause of the new problem. I will try and give some easier reproduction steps but at this stage the best I can do is give a snippet of disassembly from our product code. What follows is the unwind funclet of a problematic function: "?dtor$15@?0??open@?$QueryIteratorImpl@$0A@$0A@$00$0A@@@UEAA_KXZ@4HA": We can see that the funclet now spills the 16 bytes of xmm6 register to location rsp + 48. However the funclet has only reserved 56 bytes of stack space so it overflows by 8 bytes. My attempts to reproduce this issue with a smaller example has been unsuccessful and the code generated appears to reserve the correct amount of space to save the xmm registers. Also as a side note, I don't think the above funclet needs to save/reload the xmm6 because it doesn't modify it. I guess clang decides which registers to spill based on the parent function rather than the funclet. This results in non-optimal code rather than anything wrong. |
Hi Brain, the behavior in your example is as expected. The X86 stack is growing downward, so xmm6 is stored in [rsp + 32, rsp + 48). There's no overflow here. The math that reserves memory in stack is:
The odd pushes (pushes except 'push rbp') need an extra 8 bytes to acquire alignment. That's why we subtract 56 rather than 48 here. |
Oop, seems I was confused by the pushes behavior. The memory accessing is growing upward. For any cases the offset for xmm spilling should be 32. |
Hi Brain, I have committed a patch on https://reviews.llvm.org/D70224, I guess this should be useful for your problems. Thanks for the great tracking. |
Merged to 9.0 branch by https://reviews.llvm.org/rG3437c7fc4470. |
I have tested my product code using this latest version of clang (commit 3437c7f) and confirm it resolves my issue. Thank you very much for fixing this. |
*** Bug llvm/llvm-bugzilla-archive#43977 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#45163 has been marked as a duplicate of this bug. *** |
mentioned in issue llvm/llvm-bugzilla-archive#43977 |
mentioned in issue llvm/llvm-bugzilla-archive#44063 |
mentioned in issue llvm/llvm-bugzilla-archive#45163 |
Extended Description
I see segmentation faults in my product when switching from Clang 8 to Clang 9. Sometimes local variables are incorrectly initialised after an exception has been successfully caught. With certain input this can lead to a crash at a later point. For one of our unit tests this crash is 100% repeatable.
After some investigation I'm confident this is a Windows only issue with exception funclets incorrectly spilling XMM registers. I can confirm this issue does not occur when I use the master branch of Clang because of the change https://reviews.llvm.org/D66596 ([WinEH] Allocate space in funclets stack to save XMM CSRs). This issue is a bit of showstopper for me because it leaves me worried that there are similar problems in our product code that we have not found through testing. I'm raising this issue in the hope of getting the D66596 change incorporated into a future clang 9 release.
It's hard to come up with a simple example. The code below demonstrates the corruption problem (but not the crash). It shows how a constant data member is initialised to the wrong value.
When the following code is compiled with:
clang-cl /O2 /Ob1 /EHsc code.cpp
and run it prints "This shouldn't happen to Two, value of a is 1". I would expect the code to run without showing this message.
#include
struct One {
const size_t a = 1;
const size_t b = 0;
};
struct Two {
const size_t a = 2;
const size_t b = 0;
};
void throwIt() {
throw std::string();
}
bool mytest() {
for (int i = 0; i < 2; ++i) {
try {
One one;
throwIt();
}
catch (const std::string& s) {
return true;
}
}
return false;
}
int main(int argc, char* argv[]) {
for (int i = 0; i < 2; ++i) {
Two two;
mytest();
}
return 0;
}
For more details see annotated attached disassembly it shows how mytest() overwrites the CSR slot for XMM6 in both the unwinding and catch handling funclets.
The text was updated successfully, but these errors were encountered: