-
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
windows x64 exception data is innacurate #46803
Comments
Hi! Can you please send this patch to llvm-commits? |
I think IDA PRO must be assuming that EH labels mark instruction boundaries, but LLVM doesn't uphold that invariant. I haven't tested your patch, but I believe it would actually break exception handling, because I believe Consider this kind of input: void mayThrow();
void mayNotThrow();
void f() {
try {
mayThrow();
} catch(...) {}
mayNotThrow();
} Clang generates this code sequence: .Ltmp0:
callq "?mayThrow@@YAXXZ"
.Ltmp1:
callq "?mayNotThrow@@YAXXZ" The labels are used in the ip2state table with a one byte offset so that exceptions raised at the .Ltmp1 return address are caught. MSVC inserts a call ?mayThrow@@YAXXZ ; mayThrow
npad 1
$LN7@f:
; Line 7
call ?mayNotThrow@@YAXXZ ; mayNotThrow Inserting that extra The LLVM's Honestly, you might get more traction asking IDA for a fix. It's probably much easier for them to check if |
Hi Reid, I disagree, your assumptions are just that… I verified the output of the MS compiler first, and there is no added padding in my test output EVER. The IP2State start label in MS points to the start of an instruction not after the byte of the instruction. Case in point the first entry of the ip2state is the start of the function, then all the other items points inside an instruction. Example: Microsoft CL 2017 .xdata:00000000000150B8 $ip2state$?_main@@YAHXZ IPtoStateMap <rva loc_C1AF, -1>
.xdata:00000000000150B8 ; .rdata:$cppxdata$?_main@@YAHXZ↑o
.xdata:00000000000150C0 IPtoStateMap <rva loc_C320, 0>
.xdata:00000000000150C8 IPtoStateMap <rva loc_C348, -1>
.xdata:00000000000150D0 IPtoStateMap <rva loc_CC63, 1>
...
.text$mn:000000000000C318 mov rbx, rax
.text$mn:000000000000C31B mov [rsp+910h+var_8D0], rax
.text$mn:000000000000C31B
.text$mn:000000000000C320
.text$mn:000000000000C320 loc_C320: ; .xdata:00000000000150C0↓o
.text$mn:000000000000C320 ; try {
.text$mn:000000000000C320 xor r13d, r13d
.text$mn:000000000000C323 test rax, rax
.text$mn:000000000000C326 jz short loc_C345
.text$mn:000000000000C326
.text$mn:000000000000C328 xor eax, eax
.text$mn:000000000000C32A mov [rbx], rax
.text$mn:000000000000C32D mov [rbx+8], rax
.text$mn:000000000000C331 mov rcx, rbx ; this
.text$mn:000000000000C334 call BaseDriver::BaseDriver(void)
.text$mn:000000000000C334
.text$mn:000000000000C339 lea rax, const WinDriver::`vftable'
.text$mn:000000000000C340 mov [rbx], rax
.text$mn:000000000000C343 jmp short loc_C348
.text$mn:000000000000C343
.text$mn:000000000000C345 ; --------------------------------------------------
.text$mn:000000000000C345
.text$mn:000000000000C345 loc_C345: ; _main(void)+1B6↑j
.text$mn:000000000000C345 mov rbx, r13
.text$mn:000000000000C345 ; } // starts at C320
.text$mn:000000000000C345
.text$mn:000000000000C348
.text$mn:000000000000C348 loc_C348: ; _main(void)+1D3↑j
.text$mn:000000000000C348 ; .xdata:00000000000150C8↓o CLANG 11 rc2: .xdata:0000000000034D3C $ip2state$?_main@@YAHXZ IPtoStateMap <rva _main(void), -1>
.xdata:0000000000034D3C ; .xdata:$cppxdata$?_main@@YAHXZ↑o
.xdata:0000000000034D44 IPtoStateMap <rva loc_245FC, 0>
.xdata:0000000000034D4C IPtoStateMap <rva loc_2460B, -1>
.xdata:0000000000034D54 IPtoStateMap <rva loc_24E98, 6>
.text:00000000000245F5 xorps xmm6, xmm6
.text:00000000000245F8 movaps xmmword ptr [rax], xmm6
.text:00000000000245F8
.text:00000000000245F8 ; -----------------------------------------------------
.text:00000000000245FB db 48h
.text:00000000000245FC ; -----------------------------------------------------
.text:00000000000245FC
.text:00000000000245FC loc_245FC: ;.xdata:0000000000034D44↓o
.text:00000000000245FC ; try {
.text:00000000000245FC mov [rbp+540h], eax
.text:0000000000024602 mov rcx, rax
.text:0000000000024605 call BaseDriver::BaseDriver(void)
.text:0000000000024605
.text:0000000000024605 ; ------------------------------------------------------
.text:000000000002460A db 48h
.text:000000000002460A ; } // starts at 245FC
.text:000000000002460B ; ------------------------------------------------------
.text:000000000002460B
.text:000000000002460B loc_2460B: ; .xdata:0000000000034D4C↓o
.text:000000000002460B lea eax, const WinDriver::`vftable'
.text:0000000000024611 mov rcx, [rbp+540h]
.text:0000000000024618 mov [rcx], rax
.text:000000000002461B mov qword ptr cs:BaseDriver * driver, rcx
.text:0000000000024622 mov ecx, 8
.text:0000000000024627 call operator new(unsigned __int64) Granted the optimization applied are different, however the Best regards, |
Also to be clear, I didn't report this because of IDA output (I don't care) but because I am concerned that the exception table is wrong and would trigger the wrong EH if a problem arose at the start of the block defined in ip2state. Jerome |
I think we are in agreement: MSVC's ip2state labels always point to an instruction boundary, Clang's do not. I agree that there are no nops in your example, but it is because they are not necessary: the last instruction in the try range ( In my example, when optimizations are enabled, MSVC may insert a
LLVM only supports catching exceptions thrown from a call ( If we support non-call exceptions ( |
Hi Reid, After looking more into the output, I did notice that a nop is inserted after a call with VC. I was under the impression that it was I suppose you should close this bug then. Jerome |
This isn't the first time someone has been confused by this label+1 design choice, so IMO we can keep this open. It would be nice for these labels to point to instruction boundaries, even if it's not a correctness problem for us right now. Good luck with any attempts! There is some existing functionality for inserting nops before the epilogue to avoid similar problems, I think if you grep for SEH_Epilogue you might find it. |
@rnk the lack of nops does cause correctness problems though: #include <intrin.h>
#include <cstdio>
__declspec(noinline) void do_nothing() {}
__declspec(noinline) void test()
{
__try {
do_nothing();
} __except(1) {
printf("hello\n");
}
__debugbreak();
}
int main()
{
__try {
test();
} __except(1) {
printf("world\n");
}
return 0;
} This will infinitely print out "hello" on LLVM 14.0.0 |
Yes, I think this is a separate issue. I think it would be reasonable to add nops to functions using SEH try, since they have the ability to catch exceptions from single byte instructions such as |
I'll open a new issue for this then. In the meantime I also realized that this current issue about the start of the ranges being misaligned is actually working as intended. If the range were to start at the begin of the |
Extended Description
Hi,
While I was scrutinizing some generated code inside IDA PRO, I realized the
$ip2state
records are wrong by 1 byte... which in turn make the disassembler display the wrong start fortry[]
. This is bad, as it would dispatch the wrong exception handler if an error happens on the 1st instruction.I spent some time and found that the source of the problem is
WinException::getLabel()
used incomputeIP2StateTable
. While the function correctly build a label plus 1 for other EH table uses, incomputeIP2StateTable
it should really not add 1. So I create a new function that just does that and it resolved the issue.I attach a patch... I don't really have time to do more than this, nor can i provide a test file.
Best regards,
Jerome WITMANN
The text was updated successfully, but these errors were encountered: