This patch fix the bug PR42266. When c++ exception was nested in a
particular form, the stack was not properly unwound under Win64 due to the
layout of try map table.
Tacking what MSVC's doing, I found the 64 bits layout of try map table
is different from 32 bits. By following what MSVC's doing, this issue can
be fixed.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I don't understand why this difference exists. Beyond just trying to reproduce what MSVC does, can you explain the difference?
| llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
|---|---|---|
| 106 | It's probably better to use Triple::isArch64Bit here. I think your patch will break non-x86 Windows. | |
| llvm/test/CodeGen/X86/win-catchpad-nested-cxx.ll | ||
| 65 | This case is different than the case you are fixing, right? Can you add a test for the case in the bug report? Also, it seems the x64 case here produces the same try-map entries but with the order of the map entries reversed. Is that correct? We're probably doing too much wild card matching in the handler map, because we really need to make sure the combination of try-map entries and handler-map entries leads to the correct handler. | |
Comment Actions
Hi, Pengfei,
Is this the same bug as this patch, https://reviews.llvm.org/D79474/new/ , right?
Comment Actions
You can see that the layout of try map table of some nested exception is different under 32 and 64 bit MSVC. This patch makes LLVM has the same behavior with MSVC.
@andrew.w.kaylor I tested for below 3 cases, and get the runtime result for them in different compilers.
void ex1() {
try {
throw 1;
} catch (...) {
try {
throw 2;
} catch (...) {
}
}
}
void ex2() {
try {
try {
throw 1;
} catch (...) {
throw 2;
}
} catch (...) {
}
}
void ex3() {
try {
throw 1;
} catch (...) {
try {
try {
throw 2;
} catch (...) {
throw 3;
}
} catch (...) {
}
}
}
You can see that the layout of try map table of some nested exception is different under 32 and 64 bit MSVC. This patch makes LLVM has the same behavior with MSVC.
Comment Actions
Oh, yes. I can't believe we are doing the same work. Wish I could see your patch earlier. Do you test the 32 bits? I found the new layout didn't work on it. So I need to separate 64bits from it.
Comment Actions
Hi, Pengfei,
Did you get to the root of the bug and check the correctness of ctor&dtor ordering on your test cases?
Or you just tried to copy MSVC?
Comment Actions
No. I didn't find enough documentation that explain the mechanism of runtime. So I mainly compared to what MSVC's generation. Besides, the issue I try to fix doesn't involve ctor&dtor. So I haven't checked that.
Comment Actions
I meant did you debug into the crash of your test case in https://bugs.llvm.org/show_bug.cgi?id=42266 ?
Comment Actions
Yes. I dumped the RSP/ESP value using below code
#include <stdio.h>
#if _WIN64
unsigned long long rbp, rsp;
#define DUMP_REGS(STR, ...) \
asm("mov %%rbp, %0\nmov %%rsp, %1" : "=m"(rbp), "=m"(rsp)); \
printf(STR "\tRSP = %llx\tRBP = %llx\n", __VA_ARGS__, rsp, rbp);
#else
unsigned long ebp, esp;
#define DUMP_REGS(STR, ...) \
asm("mov %%ebp, %0\nmov %%esp, %1" : "=m"(ebp), "=m"(esp)); \
printf(STR "\tESP = %llx\tEBP = %llx\n", __VA_ARGS__, esp, ebp);
#endif
void foo()
{
DUMP_REGS("**FOO**")
try {
throw 1;
}
catch (int x) {
DUMP_REGS("Catch %d", x)
try {
try {
throw 2;
}
catch (int x) {
DUMP_REGS("Catch %d", x)
throw 3;
}
}
catch (int x) {
DUMP_REGS("Catch %d", x)
}
DUMP_REGS("Catch %d", x)
}
DUMP_REGS("FOO ret")
};
int main()
{
foo();
}The output on 64 bits is something like this
**FOO** RSP = bc583bf880 RBP = bc583bf900 Catch 1 RSP = bc583bd640 RBP = bc583bf900 Catch 2 RSP = bc583bb400 RBP = bc583bf900 Catch 3 RSP = bc583b91c0 RBP = bc583bf900 Catch 1 RSP = bc583bb400 RBP = bc583bf900 FOO ret RSP = bc583bd640 RBP = bc583bf900
Which made me guess it caused by the runtime that not set the proper stack value. Then I compared each structure LLVM generated with MSVC and found the order of try map table is the key.
Comment Actions
What I was trying to say is that you should add a new LIT test case that corresponds to the failure in PR42266. But also, I think we need stronger checking of the correlation between the try map and the handler map. Currently the test is looking for a specific order of the states in the try map, but the handler map is using a wild card check -- "CHECK-NEXT: .long "?catch${{[0-9]+}}@?0?try_in_catch@4HA"" We should be verifying somehow that the states in the try map get connected to the correct handler, which involves matching the specific handler. Your change reversed the order of the try map, but appeared not to change the handler map. That's a problem. If the handler map didn't also change, the code would be wrong.
Now that I've seen the patch from @tentzen I am not convinced that there is a required difference between 64-bit and 32-bit. I could be wrong, but it looks like the 64-bit case is different because newer runtimes require it. It isn't clear to me whether the 32-bit runtime will work with the new format.
If D79474 works for the cases you're trying to fix (including the 32-bit case), I think it's a better solution.
Comment Actions
I tested @tentzen 's patch and it failed on 32-bit in my case. I wonder if the 32-bit still using the old runtime.
Comment Actions
right, I noticed that too and looked into it further..
It turned out that Windows 32-bit runtime expects post-ordering when looking for catch-handler, but sharing the code with 64-bit runtime when looking up EH State during DTor cleaning-up.
Thus, MSVC/X86 today actually fails on Test7 in ehthrow.cxx.
Incorrect Ctor/Dtor ordering is less significant than Catch handling. for now LLVM must be compatible with MSVC.
This also affects the other two targets on Windows; Arm32 & Arm64. I re-submitted a simper and more completed patch (in D79474), including test case for AArch64. will add one for Arm32 later..
clang-format not found in user's PATH; not linting file.