[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337312: The semantics of DW_CFA_GNU_args_size have changed
subtile over the (authored by joerg, committed by ).
Herald added subscribers: llvm-commits, christof.
Changed prior to commit:
https://reviews.llvm.org/D38680?vs=150049=155943#toc
Repository:
rL LLVM
https://reviews.llvm.org/D38680
Files:
libunwind/trunk/src/UnwindCursor.hpp
libunwind/trunk/src/libunwind.cpp
Index: libunwind/trunk/src/UnwindCursor.hpp
===
--- libunwind/trunk/src/UnwindCursor.hpp
+++ libunwind/trunk/src/UnwindCursor.hpp
@@ -1411,8 +1411,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
Index: libunwind/trunk/src/libunwind.cpp
===
--- libunwind/trunk/src/libunwind.cpp
+++ libunwind/trunk/src/libunwind.cpp
@@ -188,8 +188,20 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ // First, get the FDE for the old location and then update it.
+ co->getInfo();
co->setInfoBasedOnIPRegister(false);
+ // If the original call expects stack adjustment, perform this now.
+ // Normal frame unwinding would have included the offset already in the
+ // CFA computation.
+ // Note: for PA-RISC and other platforms where the stack grows up,
+ // this should actually be - info.gp. LLVM doesn't currently support
+ // any such platforms and Clang doesn't export a macro for them.
+ if (info.gp)
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + info.gp);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
Index: libunwind/trunk/src/UnwindCursor.hpp
===
--- libunwind/trunk/src/UnwindCursor.hpp
+++ libunwind/trunk/src/UnwindCursor.hpp
@@ -1411,8 +1411,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
Index: libunwind/trunk/src/libunwind.cpp
===
--- libunwind/trunk/src/libunwind.cpp
+++ libunwind/trunk/src/libunwind.cpp
@@ -188,8 +188,20 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ // First, get the FDE for the old location and then update it.
+ co->getInfo();
co->setInfoBasedOnIPRegister(false);
+ // If the original call expects stack adjustment, perform this now.
+ // Normal frame unwinding would have included the offset already in the
+ // CFA computation.
+ // Note: for PA-RISC and other platforms where the stack grows up,
+ // this should actually be - info.gp. LLVM doesn't currently support
+ // any such platforms and Clang doesn't export a macro for them.
+ if (info.gp)
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + info.gp);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. Ping @joerg https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
rnk added a comment. In https://reviews.llvm.org/D38680#1123018, @joerg wrote: > After a careful review of newer GCC / libgcc and the assembler annotations > from LLVM, I have come to the following conclusions: > > (1) The semantics have been somewhat changed by GCC in recent years. There is > no actual specification, so we have to go by what behavior actually makes > sense. > (2) The primary motivation is still that the DW_CFA_GNU_args_size is a > call-site specific annotation. It is expected to be applied when the IP is > moved by the personality routine to compensate for the call site specific > (temporary) adjustment. Right. > (3) It is not clear with plain unw_set_ip outside the scope of the Itanium EH > handling should have this behavior, so it might need to be split into an > internal routine. I don't know enough about this code to really respond to this. > (4) LLVM does not produce correct CFA annotation for stdcall and similar > cases where the callee removes additional stack space. Here's what we generate for that case today: https://godbolt.org/g/33cNJy The important part is: .cfi_escape 0x2e, 0x0c pushl $3 .cfi_adjust_cfa_offset 4 pushl $2 .cfi_adjust_cfa_offset 4 pushl $1 .cfi_adjust_cfa_offset 4 calll __Z13may_throw_stdiii@12 .cfi_adjust_cfa_offset -12 Are you saying that the runtime will calculate the wrong CFA because it will include the `.cfi_adjust_cfa_offset -12`? As in, adding a nop after the call would fix the glitch? If so, I think the right thing to do would be to fix libunwind to use return_address - 1 when unwinding. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
compnerd accepted this revision. compnerd added a comment. This revision is now accepted and ready to land. This makes much more sense. Thanks @joerg https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo accepted this revision. mstorsjo added a comment. Sounds sensible to me; I've tested the patch in the setup that showed the issue this original patch tried to fix, and it seems to work fine there. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
joerg updated this revision to Diff 150049.
joerg added a comment.
After a careful review of newer GCC / libgcc and the assembler annotations from
LLVM, I have come to the following conclusions:
(1) The semantics have been somewhat changed by GCC in recent years. There is
no actual specification, so we have to go by what behavior actually makes sense.
(2) The primary motivation is still that the DW_CFA_GNU_args_size is a
call-site specific annotation. It is expected to be applied when the IP is
moved by the personality routine to compensate for the call site specific
(temporary) adjustment.
(3) It is not clear with plain unw_set_ip outside the scope of the Itanium EH
handling should have this behavior, so it might need to be split into an
internal routine.
(4) LLVM does not produce correct CFA annotation for stdcall and similar cases
where the callee removes additional stack space.
The patch covers the first two items.
https://reviews.llvm.org/D38680
Files:
src/UnwindCursor.hpp
src/libunwind.cpp
Index: src/libunwind.cpp
===
--- src/libunwind.cpp
+++ src/libunwind.cpp
@@ -188,8 +188,13 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ co->getInfo();
co->setInfoBasedOnIPRegister(false);
+ if (info.gp)
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + info.gp);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
Index: src/UnwindCursor.hpp
===
--- src/UnwindCursor.hpp
+++ src/UnwindCursor.hpp
@@ -1411,8 +1411,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
Index: src/libunwind.cpp
===
--- src/libunwind.cpp
+++ src/libunwind.cpp
@@ -188,8 +188,13 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ co->getInfo();
co->setInfoBasedOnIPRegister(false);
+ if (info.gp)
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + info.gp);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
Index: src/UnwindCursor.hpp
===
--- src/UnwindCursor.hpp
+++ src/UnwindCursor.hpp
@@ -1411,8 +1411,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. In https://reviews.llvm.org/D38680#1077262, @joerg wrote: > I'm back to the point where I can't reproduce the problem :( Can we start > providing an actual failing test case? It's annoying to debug a problem when > you can't reproduce it. My testcase that triggers the issue is available at https://martin.st/temp/hello-exception.cpp. With this input file, I can produce the DW_CFA_GNU_args_size opcodes with this command: `clang -target i386-netbsd -O3 -fomit-frame-pointer -S -o - hello-exception.cpp` (tested with both clang 4.0 and current trunk). https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
joerg added a comment. I'm back to the point where I can't reproduce the problem :( Can we start providing an actual failing test case? It's annoying to debug a problem when you can't reproduce it. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. Herald added a subscriber: chrib. Ping @joerg and @rnk - what's a sensible path forward with respect to this? @rnk thinks this change is correct, while @joerg disagrees, but hasn't had time to look into it yet. Is it acceptable to apply this now and then revert later once @joerg has analyzed the issue? https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. Ping @joerg https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
rnk added a comment. In https://reviews.llvm.org/D38680#903203, @joerg wrote: > I've looked at this in some detail now. I'm not exactly sure yet why it is > broken. The patch seems quite wrong to me. DW_CFA_GNU_args_size should be > applied only when unwinding a call instruction and that regard, the commit > message of the original change is quite correct. What I am still trying to > understand is how the precise unwind frame disagrees with the unwinder. If you look at Clang's output here https://godbolt.org/g/jFcSxz, you can see that we emit precise CFA adjustments for each push. We don't need to adjust the CFA by gnu arg size in UnwindCursor::step, which unwinds through frames. We only apply it when setting up the register context before transitioning to the landingpad. That's why unw_set_reg UNW_REG_IP is at least approximately the right place to do this SP adjustment, IMO. Basically, a general purpose unwinder that collects return addresses can ignore gnu args size because we already have CFA adjustments, but an unwinder that implements landingpad transitions must adjust SP by gnu arg size as part of that context switch. At least, that's how I understand this change. Maybe older (VAX?) compilers didn't implement gnu arg size this way, but at least with this patch, we handshake with ourselves correctly. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. In https://reviews.llvm.org/D38680#903203, @joerg wrote: > I've looked at this in some detail now. I'm not exactly sure yet why it is > broken. The patch seems quite wrong to me. DW_CFA_GNU_args_size should be > applied only when unwinding a call instruction and that regard, the commit > message of the original change is quite correct. What I am still trying to > understand is how the precise unwind frame disagrees with the unwinder. @joerg Any update on this matter, how should we move forward with unbreaking the unwinding on i386? https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
joerg added a comment. I've looked at this in some detail now. I'm not exactly sure yet why it is broken. The patch seems quite wrong to me. DW_CFA_GNU_args_size should be applied only when unwinding a call instruction and that regard, the commit message of the original change is quite correct. What I am still trying to understand is how the precise unwind frame disagrees with the unwinder. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment. In https://reviews.llvm.org/D38680#901892, @dtzWill wrote: > Wow, I've finally debugged some unwind failures to this on x86_64 Linux ... > > 1. Call-frame optimization introduces it > 2. This fixes the cases I was investigating!! That's good to hear! FWIW, I've talked to @joerg off-list and he said he'll try to look into it soon. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
dtzWill added a comment. Wow, I've finally debugged some unwind failures to this on x86_64 Linux ... 1. Call-frame optimization introduces it 2. This fixes the cases I was investigating!! Thank you! https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
rnk accepted this revision. rnk added a comment. lgtm Comment at: src/libunwind.cpp:188 + co->getInfo(); + pint_t orgArgSize = (pint_t)info.gp; + uint64_t orgFuncStart = info.start_ip; mstorsjo wrote: > rnk wrote: > > I think it makes sense to have this here: the contract is that if the > > personality sets the IP when the cursor pointed to a PC with a non-zero arg > > size, we should adjust SP for the personality. > > > > However, it's not clear to me that we don't need the same adjustment when > > stepping across frames that use arg size without a frame pointer. > When stepping across frames, the gnu args size is already included in, as > part of the CFA offset, so with the current code, it steps too far. OK, yes, now I see the code that does that. :) https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added inline comments. Comment at: src/libunwind.cpp:188 + co->getInfo(); + pint_t orgArgSize = (pint_t)info.gp; + uint64_t orgFuncStart = info.start_ip; rnk wrote: > I think it makes sense to have this here: the contract is that if the > personality sets the IP when the cursor pointed to a PC with a non-zero arg > size, we should adjust SP for the personality. > > However, it's not clear to me that we don't need the same adjustment when > stepping across frames that use arg size without a frame pointer. When stepping across frames, the gnu args size is already included in, as part of the CFA offset, so with the current code, it steps too far. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
rnk added inline comments. Comment at: src/libunwind.cpp:188 + co->getInfo(); + pint_t orgArgSize = (pint_t)info.gp; + uint64_t orgFuncStart = info.start_ip; I think it makes sense to have this here: the contract is that if the personality sets the IP when the cursor pointed to a PC with a non-zero arg size, we should adjust SP for the personality. However, it's not clear to me that we don't need the same adjustment when stepping across frames that use arg size without a frame pointer. https://reviews.llvm.org/D38680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.
I think that the problem is that we are using the generic register name, but we
need to use the target specific register name. On x86, EIP/ESP are swapped.
We should also have a test case for this. I had reduced this down to a simpler
test case of:
void f(int,int,int,int,int,int,int,int,int);
int main() {
try {
f(0,1,2,3,4,5,6,7,8);
} catch (int) {
return 0;
}
return 1;
}
https://reviews.llvm.org/D38680
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo added a comment.
In https://reviews.llvm.org/D38680#892487, @compnerd wrote:
> I think that the problem is that we are using the generic register name, but
> we need to use the target specific register name. On x86, EIP/ESP are
> swapped.
You mean EBP/ESP? I think the code here does the right thing, with `UNW_REG_SP`
always mapping to the actual ESP.
> We should also have a test case for this. I had reduced this down to a
> simpler test case of:
>
> void f(int,int,int,int,int,int,int,int,int);
>
> int main() {
> try {
> f(0,1,2,3,4,5,6,7,8);
> } catch (int) {
> return 0;
> }
> return 1;
> }
>
Oh, great if you can reproduce the issue with that! Feel free to try to dig
into the issue and figure out a better fix then.
https://reviews.llvm.org/D38680
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size
mstorsjo created this revision.
This effectively reverts SVN r204290 and r204292 (back when this code was part
of libcxxabi).
According to SVN r204290, the primary architecture using the
DW_CFA_GNU_args_size opcode is VAX.
However, clang also produces it on X86 when it has done
X86CallFrameOptimization, which gets done much more frequently if the stack is
aligned to 4 bytes (which is the default when targeting windows).
This issue can be tested by building code for x86 linux (at least for 32 bit)
with clang with -mstack-alignment=4.
I'm not sure if this code should be handled differently for VAX with some
ifdef, or what the correct interpretation of this opcode is. I ran into this
issue while trying to use libunwind for 32 bit windows (where clang uses a 4
byte stack alignment by default), found this commit, and noticed I got it
working by reverting it. And later noticed I could reproduce the same issue on
32 bit x86 linux as well, by forcing -mstack-alignment=4.
https://reviews.llvm.org/D38680
Files:
src/UnwindCursor.hpp
src/libunwind.cpp
Index: src/libunwind.cpp
===
--- src/libunwind.cpp
+++ src/libunwind.cpp
@@ -182,8 +182,16 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ co->getInfo();
+ pint_t orgArgSize = (pint_t)info.gp;
+ uint64_t orgFuncStart = info.start_ip;
co->setInfoBasedOnIPRegister(false);
+ // and adjust REG_SP if there was a DW_CFA_GNU_args_size
+ if ((orgFuncStart == info.start_ip) && (orgArgSize != 0))
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + orgArgSize);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
Index: src/UnwindCursor.hpp
===
--- src/UnwindCursor.hpp
+++ src/UnwindCursor.hpp
@@ -1356,8 +1356,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
Index: src/libunwind.cpp
===
--- src/libunwind.cpp
+++ src/libunwind.cpp
@@ -182,8 +182,16 @@
co->setReg(regNum, (pint_t)value);
// specical case altering IP to re-find info (being called by personality
// function)
-if (regNum == UNW_REG_IP)
+if (regNum == UNW_REG_IP) {
+ unw_proc_info_t info;
+ co->getInfo();
+ pint_t orgArgSize = (pint_t)info.gp;
+ uint64_t orgFuncStart = info.start_ip;
co->setInfoBasedOnIPRegister(false);
+ // and adjust REG_SP if there was a DW_CFA_GNU_args_size
+ if ((orgFuncStart == info.start_ip) && (orgArgSize != 0))
+co->setReg(UNW_REG_SP, co->getReg(UNW_REG_SP) + orgArgSize);
+}
return UNW_ESUCCESS;
}
return UNW_EBADREG;
Index: src/UnwindCursor.hpp
===
--- src/UnwindCursor.hpp
+++ src/UnwindCursor.hpp
@@ -1356,8 +1356,6 @@
this->setInfoBasedOnIPRegister(true);
if (_unwindInfoMissing)
return UNW_STEP_END;
-if (_info.gp)
- setReg(UNW_REG_SP, getReg(UNW_REG_SP) + _info.gp);
}
return result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
