Page MenuHomePhabricator

[WinEH64] Fix a crush issue when c++ exception nested in a particular form.
AbandonedPublic

Authored by pengfei on May 11 2020, 7:28 PM.

Details

Summary

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.

Diff Detail

Event Timeline

pengfei created this revision.May 11 2020, 7:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 7:28 PM

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.

Hi, Pengfei,
Is this the same bug as this patch, https://reviews.llvm.org/D79474/new/ , right?

@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.

pengfei added a comment.EditedMay 12 2020, 8:39 PM

Hi, Pengfei,
Is this the same bug as this patch, https://reviews.llvm.org/D79474/new/ , right?

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.

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?

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?

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.

I meant did you debug into the crash of your test case in https://bugs.llvm.org/show_bug.cgi?id=42266 ?

I meant did you debug into the crash of your test case in https://bugs.llvm.org/show_bug.cgi?id=42266 ?

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.

@andrew.w.kaylor I tested for below 3 cases, and get the runtime result for them in different compilers.

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.

@andrew.w.kaylor I tested for below 3 cases, and get the runtime result for them in different compilers.

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.

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.

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..

pengfei abandoned this revision.May 13 2020, 11:54 PM

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..

Great! Thank you. I'll abandon this now.

peterjc123 removed a subscriber: peterjc123.