[libunwind,v12.0.1] : unw_step() fails without this patch to set _unwindInfoMissing in UnwindCursor.hpp

[JVD66]

Without the attached patch, which simply sets the cursor _unwindInfoMissing boolean to false
when valid info is found, to the source from 'libunwind-12.0.1-src.tar.xz' ,
all calls to unw_step() fail, even if valid unwind info was found, on any of the platforms:
x86_64-redhat-linux (Fedora Core Linux 34),
aarch64-unknown-linux-android2{4,6,8,9} , and arm-unknown-linux-androideabi2{4,6,8,9} .

Without the patch, calls to unw_step() ALWAYS return UNW_STEP_END (0).
With the patch, the same code that expects UNW_STEP_SUCCESS values from unw_step()
now gets them, and prints full stack traces.

libunwind-12.0.1.patch.txt

It took me a long time to find and correct this bug !

Nothing in UnwindCursor.hpp was setting _unwindInfoMissing to false, when valid info was found !

[DimitryAndric]

As far as I can see, the _unwindInfoMissing member of the template class UnwindCursor is always initialized to false in the corresponding constructors:

% grep -A1 ::UnwindCursor libunwind/src/UnwindCursor.hpp
UnwindCursor<A, R>::UnwindCursor(unw_context_t *context, A &as)
    : _addressSpace(as), _unwindInfoMissing(false) {
--
UnwindCursor<A, R>::UnwindCursor(CONTEXT *context, A &as)
    : _addressSpace(as), _unwindInfoMissing(false) {
--
UnwindCursor<A, R>::UnwindCursor(unw_context_t *context, A &as)
    : _addressSpace(as), _registers(context), _unwindInfoMissing(false),
--
UnwindCursor<A, R>::UnwindCursor(A &as, void *)
    : _addressSpace(as), _unwindInfoMissing(false), _isSignalFrame(false) {

But is this during a state where it first concludes that it isn't able to find unwind info, and sets _unwindInfoMissing to true, but then later changes its mind?

Do you have some sort of test case that can be run to show the issue?

[JVD66]

OK, here is a minimal test program that just reproduces the issue (ONLY!), and a more complete patch:

test_54799.tar.gz

$ gunzip < test_54799.tar.gz | tar -xpf -
$ make UNW_12_DIST=${directory_where_unpatched_libunwind_12.0.1_is_installed} \\
          UNW_FIXED_DIST=${directory_where_PATCHED_libunwind_is_installed}


eg. I have my patched libunwind installed in /usr/local/{lib64,include}, and a build of the
unpatched libunwind v12.0.1 installed under ./libunwind/usr/local/{lib64,include} :

$ make UNW_12_DIST=libunwind/usr/local UNW_FIXED_DIST=/usr/local
clang -Ilibunwind/usr/local/include/libunwind t_unw.c -o t_unw_broken libunwind/usr/local/lib64/libunwind.a -lrt -ldl -lpthread -lm -lc
clang -I/usr/local/include/libunwind t_unw.c -o t_unw_fixed /usr/local/lib64/libunwind.a -lrt -ldl -lpthread -lm -lc

$ ./t_unw_broken
sighandler: #000 PC:   0x7f0933b23795 SP:   0x7fff1de32278

$ ./t_unw_fixed
sighandler: #000 PC:   0x7fa7fbcc3795 SP:   0x7ffc2bfb9588

sighandler: #001 PC:         0x4013a9 SP:   0x7ffc2bfb9590

sighandler: #000 PC:         0x403af5 SP:   0x7ffc2bfb7fd0

sighandler: #001 PC:         0x402653 SP:   0x7ffc2bfb7fe0

sighandler: #002 PC:         0x40203e SP:   0x7ffc2bfb83d0

sighandler: #003 PC:         0x40137a SP:   0x7ffc2bfb83f0

sighandler: #004 PC:   0x7fa7fbe89a20 SP:   0x7ffc2bfb8f00

sighandler: #005 PC:         0x4013a9 SP:   0x7ffc2bfb9590

sighandler: #006 PC:         0x4013b9 SP:   0x7ffc2bfb95b0

sighandler: #007 PC:         0x4013c9 SP:   0x7ffc2bfb95c0

sighandler: #008 PC:         0x40149b SP:   0x7ffc2bfb95d0

sighandler: #009 PC:   0x7fa7fbb8ab75 SP:   0x7ffc2bfb96a0

sighandler: #010 PC:         0x40118e SP:   0x7ffc2bfb9770

See, without my patch, unw_step NEVER succeeds, because _unwindInfoMissing is never set to false.

Improved Patch:

libunwind-12.0.1.patch.txt

[DimitryAndric]

I can confirm that this works with main as of c807141. E.g. before:

$ ./t_unw-before
sighandler: #000 PC:   0x7effc8b4997d SP:   0x7ffd6871b4e8

$ ./t_unw-after
sighandler: #000 PC:   0x7f335832e97d SP:   0x7ffceae1dbe8

sighandler: #001 PC:   0x5559d8a343d9 SP:   0x7ffceae1dbf0

sighandler: #000 PC:   0x5559d8a37b6e SP:   0x7ffceae1c5f0

sighandler: #001 PC:   0x5559d8a35f23 SP:   0x7ffceae1c600

sighandler: #002 PC:   0x5559d8a358ee SP:   0x7ffceae1ca10

sighandler: #003 PC:   0x5559d8a343a6 SP:   0x7ffceae1ca30

sighandler: #004 PC:   0x7f33581d3520 SP:   0x7ffceae1d540

sighandler: #005 PC:   0x5559d8a343d9 SP:   0x7ffceae1dbf0

sighandler: #006 PC:   0x5559d8a343e9 SP:   0x7ffceae1dc10

sighandler: #007 PC:   0x5559d8a343f9 SP:   0x7ffceae1dc20

sighandler: #008 PC:   0x5559d8a344c4 SP:   0x7ffceae1dc30

sighandler: #009 PC:   0x7f33581bad90 SP:   0x7ffceae1dd00

sighandler: #010 PC:   0x7f33581bae40 SP:   0x7ffceae1dda0

sighandler: #011 PC:   0x5559d8a341b5 SP:   0x7ffceae1ddf0

This is probably best handled by a review on Phabricator, though. I'll try to find some people that might be considered code owners. :)

[DimitryAndric]

Reviewing here: https://reviews.llvm.org/D123427

@JVD66 please note that there is very little chance of this landing in the release/12.x branch, as it is no longer maintained. It should first go into main, then it is likely that we can merge it into 14.0.1.

[MaskRay]

The test test_54799.tar.gz is incorrect.

Quote nongnu libunwind's manpage,

The machine-state is expected to be one provided by a call to unw_getcontext();

unw_getcontext is missing from the test, so unw_cursor_t has uninitialized values.
If you read its src/x86_64/init.h:common_init, unw_init_local may fail with a negative return value and some DWARF states may not be initialized.

For the signal frame, we should use nongnu libunwind's unw_init_local2 with UNW_INIT_SIGNAL_FRAME.
llvm-project libunwind does not implement it but that is benign since the initial frame's PC is just offset by one, which isn't a big problem.

By tracing through nongnu libunwind's logic, I believe unw_set_reg(&u_curs, UNW_REG_IP, (unw_word_t)r->rip); does not reset DWARF states,
so it is incorrect to assume that this SHOULD cause _unwindInfoMissing to be set to false!.

Second, the two lines change the sigcontext registers:

      unw_get_reg(&u_curs, UNW_REG_IP, ((unw_word_t*)&(r->rip)));
      unw_get_reg(&u_curs, UNW_REG_SP, ((unw_word_t*)&(r->rsp)));

I think two local variables are needed.
On Linux glibc, modifying the sigcontext registers change the saved rip/rsp in the __restore_rt frame and will cause unwinding from __restore_rt to the same frame.
With llvm-project libunwind the program will enter an infinite loop.
With nongnu libunwind, the loop will exit because there is a special case if (c->ip == prev_ip && c->cfa == prev_cfa) in src/dwarf/Gparser.c.
The condition seems generally unneeded.

Last, the loop exit condition should be } while(unw_step(&u_curs) > 0);: the function may return a negative value if DWARF call frame information is missing.

[JVD66]

@MaskRay: thanks for your interesting & informative previous comment - but it raises several issues:

1. Where is the 'nongnu libunwind's manpage' to which you refer ?
   There is no manual page I can find in the 'docs' directory of the distro in the libunwind-12.0.1.src.tar.xz
   I downloaded - the docs directory just contains build instructions .
   A manual page or some usage documentation would be greatly appreciated and its lack is one of the main
   deficiencies of the distro IMHO .

2. unw_getcontext() (implemented in UnwindRegistersSave.S) creates a NEW context from the callers' state.
   This is not what I want in this case - I want to only examine the context created by the system and passed
   to my SIG{SEGV,BUS...} signal handler. If parts of the cursor structure are not initialized properly by
   a call to 'unw_init_local', then that is a bug . I did try using '_Unwind_Backtrace' at first, but this
   just reports on the context of the caller, not on the context supplied to my signal handler - this is not
   what is needed here.
   THERE IS NO FUNCTION CALLED 'unw_init_local2' in the distro!
   What distro are you referring to ?
   The subject of this bug is only the distro shipped in 'libunwind-12.0.1.src.tar.xz', which does not contain
   any unw_init_local2 call.

3. RE:

Second, the two lines change the sigcontext registers:
unw_get_reg(&u_curs, UNW_REG_IP, ((unw_word_t*)&(r->rip)));
unw_get_reg(&u_curs, UNW_REG_SP, ((unw_word_t*)&(r->rsp)));

No, they do not - the previous line is :
r = &new_regs;
which sets r to point to a new 'struct sigcontext' on the stack, whose value is initialized to all 0's.

Again, my copy of llvm-project libunwind has NO 'src/dwarf/Gparser.c' file, so I don't know
what software you are referring to here .

4. RE:

   I believe unw_set_reg(&u_curs, UNW_REG_IP, (unw_word_t)r->rip); does not reset DWARF states,
   so it is incorrect to assume that this SHOULD cause _unwindInfoMissing to be set to false!.

   I don't see how you can read the UnwindCursor<A, R>::setInfoBasedOnIPRegister() function and say that -
   the whole point is that the IP register is the only way the code has of knowing where the context shows
   the code is , and is the basis of all its text symbol lookups.
   If I had done :

              unw_set_reg(&u_curs, UNW_REG_IP,  r->rip);
              unw_set_reg(&u_curs, UNW_REG_SP, r->rsp);
   

   Then the unmodified libunwind may have worked, since setInforBasedOnIPRegister() would have got in first,
   setting _unwindInfoMissing to false, but since I did it in the opposite order:

               unw_set_reg(&u_curs, UNW_REG_SP,  r->rsp);
               unw_set_reg(&u_curs, UNW_REG_IP, r->rip);
   

   _unwindInfoMissing gets set to true by the first call, and nothing sets it to false in the second call.
   My point is that setting _unwindInfoMissing should not depend on the order of initialization of the
   cursor registers .

5. If you read 'UnwindCursor<A, R>::step()' , you will see that its two possible return values are:
   A) UNW_STEP_SUCCESS (1) or
   B) UNW_STEP_STOP (0)
   so there is no need to test for a negative return value.

Please , can we keep this discussion limited to the code I raised the bug about, which is the
llvm-project's libunwind v12.0.1 - not any other code .

Yes, please, ship some usage document or manual page with llvm-project libunwind, that
would have cleared up many of these issues.

[MaskRay]

1. Where is the 'nongnu libunwind's manpage' to which you refer ?
   There is no manual page I can find in the 'docs' directory of the distro in the libunwind-12.0.1.src.tar.xz
   I downloaded - the docs directory just contains build instructions .
   A manual page or some usage documentation would be greatly appreciated and its lack is one of the main
   deficiencies of the distro IMHO .

https://www.nongnu.org/libunwind/docs.html

llvm-project libunwind emulates a subset of its functionality. There is no manpage yet.

unw_getcontext() (implemented in UnwindRegistersSave.S) creates a NEW context from the callers' state.
This is not what I want in this case - I want to only examine the context created by the system and passed
to my SIG{SEGV,BUS...} signal handler. If parts of the cursor structure are not initialized properly by
a call to 'unw_init_local', then that is a bug . I did try using '_Unwind_Backtrace' at first, but this
just reports on the context of the caller, not on the context supplied to my signal handler - this is not
what is needed here.

Please , can we keep this discussion limited to the code I raised the bug about, which is the
llvm-project's libunwind v12.0.1 - not any other code .

Yes, please, ship some usage document or manual page with llvm-project libunwind, that
would have cleared up many of these issues.

(A bug not reproducible in latest trunk or release will be closed.
You need to check the behavior with 14.0.0, though I think it is identical to 12.0.1.)

We need to confirm whether the issue indicates a bug first.
I am unsure keeping the unw_context_t uninitialized and just manipulating UNW_REG_IP UNW_REG_SP before unw_init_local qualifies a bug.

THERE IS NO FUNCTION CALLED 'unw_init_local2' in the distro!
What distro are you referring to ?
The subject of this bug is only the distro shipped in 'libunwind-12.0.1.src.tar.xz', which does not contain
any unw_init_local2 call.

I have mentioned that llvm-project libunwind doesn't implement unw_init_local2.
In nongnu libunwind, the identifier is a macro expanding to _Ux86_64_init_local2.

RE:

Second, the two lines change the sigcontext registers:
unw_get_reg(&u_curs, UNW_REG_IP, ((unw_word_t*)&(r->rip)));
unw_get_reg(&u_curs, UNW_REG_SP, ((unw_word_t*)&(r->rsp)));
No, they do not - the previous line is :
r = &new_regs;
which sets r to point to a new 'struct sigcontext' on the stack, whose value is initialized to all 0's.

I don't see r = &new_regs; in the unextracted test_54799.tar.gz.
It tries to overwrite the ucontext_t structure.

Again, my copy of llvm-project libunwind has NO 'src/dwarf/Gparser.c' file, so I don't know
what software you are referring to here .

https://www.nongnu.org/libunwind/ https://github.com/libunwind/libunwind

[JVD66]

@MaskRay : Thanks for your comments.

Sorry, please clarify:

I am unsure keeping the unw_context_t uninitialized and just manipulating UNW_REG_IP UNW_REG_SP before unw_init_local qualifies a bug.

In what way is the code in t_unw.c using an 'uninitialized' unw_cursor_t ?

It does explicitly call 'unw_init_local( &unw_curs, &unw_ctx )' before any use of any unw_* function
that uses an unw_cursor_t* or unw_context_t* .

So where does it go wrong ?

In the real version that I use in my company's application (a much larger C++ class wrapper around the unw_*
calls and unw_step()) I do test for :
if ( step_ok( unw_step() ) && (previous_stack_pointer() != current_stack_pointer()))
{ continue_loop = true ; ...
}else
break;

(pseudo-code) .

But I found that without my patch, it never got past the first stack frame;
this is because I was setting ALL REGISTERS in the (unw_init_local() initialized unw_cursor_t*)
before I set SP , and then FINALLY, PC / IP - the somehow arbitrarily "wrong" order according
to libunwind - but if I had not set SP, and just IP, then how could setInfoBasedOnIPRegister() possibly
get the EH & CFI headers and not set _UnwindInfoMissing to true? And once set to true, nothing
seems to set it to false - so I don't see how LLVM libunwind v12 would print more than one
stack frame - maybe I should have used the unw_init_local2 macro - will investigate.
This is an implementation for Android aarch64 or armv7a only, and works great ;
it has to use Clang , not GCC , which is why I am focusing only on LLVM's libunwind.
I could not get it to work without my patch.
I replace the Android sysroot's libunwind (for some architectures only, in some android versions)
with my build , and specify which compiler to build with, and everything works - I get full stack
traces, and dladdr() works for exported symbols and to resolve the load module name for function
symbols found using unw_get_proc_info() or _Unwind_Get_FDE() .
But, with either Android's stock Clang libunwind, or my build of 12.0.1 without my patch,
only the FIRST stack frame is printed for a signal stack.
Notice that my test program gets 2 SIGSEGVs: once intentionally by my program and once
internally (I think raised by libunwind's unw_step()) when it does a unw_getcontext() ?
but that's OK, it does print the address of the function that called the function that
raised the signal, which is all I want, and we are catching SIGSEGV!
This does not happen in my 't_bts.C' C++ test program, which raises a sigsegv, and prints:
Main is 0x406860
t_bts: no args - causing SIGSEGV!
sighandler_func: SIGSEGV(11) [pid:806544,tid:806544] <- [pid:806544,tid:806544] uucontext 0x7f3b781cf9c0 fault @ (nil) PC: 0x7f3b7857c795 SP: 0x7ffd63453168 .
backtrace: @1649792572s.965847090ns : SIGSEGV(11) [pid:806544,tid:806544] <- [pid:0,tid:806544] @ (nil) [PC: 0x7f3b7857c795 SP: 0x7ffd63453168] ucontext : 0x7f3b781cf9c0
0 PC: 0X7F3B7857C795 FN: 0X7F3B7857C780 ('/lib64/libc.so.6@0x7f3b7857c780')
1 PC: 0X40680D FN: 0X4067F0 ('./t_bts@0x4067f0')
2 PC: 0X406835 FN: 0X406820 ('./t_bts@0x406820')
3 PC: 0X406855 FN: 0X406840 ('./t_bts@0x406840')
4 PC: 0X4068C5 FN: 0X406860 ('./t_bts@0x406860')
5 PC: 0X7F3B78443B75 FN: 0X7F3B78443AA0
6 PC: 0X40232E FN: 0X402300 ('./t_bts@0x402300')
signal_handler: Sent StackDump & Restart Intents to Java OK.

To get this to work, I have to patch and build libunwind with the v12.0.8 Clang++ compiler used by Android .

The t_unw.c test program from is just a demo of the issue I had to fix.

This is the t_unw.c I thought I included in the test tarball:

t_unw.c.txt

[JVD66]

(unfortuntely, since there are no exported symbols in my test program, and __strlen_avx2() (0X7F3B7857C795) is a weak symbol, dladdr() can't resolve any addresses to names, but on Android aarch64 and armv7a it with the '-a'
argument to send itself SIGABRT (also caught), the signal handler prints 'tgkill' for the exported Bionic Libc symbol.
Without my patch, the address of the function calling 'tgkill' is not printed.
).
Sorry I can't share my full C++ 'BackTraceState.H' and 'backtrace.cpp' and t_bts.C files with you,
they are protected by company copyright!