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:
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
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
>
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
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
___
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
21 matches
Mail list logo
