[libunwind] Adjust the signal_frame test for Arm

[miyuki]

Summary

This patch adjusts the signal_frame.pass.cpp to pass on Arm targets:

• When Arm EHABI is used the unwinder does not use DWARF, hence the DWARF-specific check unw_is_signal_frame() must be disabled.
• Certain C libraries don't include EH tables, so the unwinder must not try to step out of main(). The patch moves the test code out of main() into a separate function to avoid this.

[broadwaylamb]

This test won't compile for ARM Linux (using ARM EHABI), I get the error this directive must appear between .cfi_startproc and .cfi_endproc directives.

Adding those directives fixes the error (and the test successfully passes), but I'm not a libunwind expert and not sure whether it's ok. Do you think explicitly adding those directives is appropriate? Does it even make sense to run this test with ARM EHABI enabled?

[steven_wu]

In D70397#1753428, @broadwaylamb wrote:

This test won't compile for ARM Linux (using ARM EHABI), I get the error this directive must appear between .cfi_startproc and .cfi_endproc directives.

Can you post the compile commands used by the test? .cfi_startproc and .cfi_endproc should be there for most platforms if the test is built with -funwind-table.

Adding those directives fixes the error (and the test successfully passes), but I'm not a libunwind expert and not sure whether it's ok. Do you think explicitly adding those directives is appropriate? Does it even make sense to run this test with ARM EHABI enabled?

I think this is not the correct fix. We should probably disable this test for ARM EHABI and APPLE (which uses compact unwind) that doesn't use DWARF unwind. In this patch, add another stack frame completely defeat the purpose of assert(unw_step(&cursor) > 0);.

[miyuki]

In D70397#1753767, @steven_wu wrote:

In this patch, add another stack frame completely defeat the purpose of assert(unw_step(&cursor) > 0);.

Is the DWARF unwinder supposed to be always able to step out of main()?

[steven_wu]

In D70397#1753789, @miyuki wrote:

In D70397#1753767, @steven_wu wrote:

In this patch, add another stack frame completely defeat the purpose of assert(unw_step(&cursor) > 0);.

Is the DWARF unwinder supposed to be always able to step out of main()?

I don't know signal frame but I think the purpose of that assert is to check unw_step return value > 0 from main if that is a signal frame. Adding another frame will make that assert always be true, regardless of signal frame, thus it is not testing signal frame anymore.

[broadwaylamb]

In D70397#1753767, @steven_wu wrote:

Can you post the compile commands used by the test? .cfi_startproc and .cfi_endproc should be there for most platforms if the test is built with -funwind-table.

I'm cross-compiling on Windows for ARM Linux (arm-linux-gnueabihf), so I'm using ARM EHABI.

Here's the complete command (I've reduced the number of flags from the actual test invocation, but believe me, those were unrelated, and the error was still produced there):

bin\clang++.exe -funwind-tables -I..\libunwind\include -S ..\libunwind\test\signal_frame.pass.cpp

It produces the error. And indeed, if I remove the asm(".cfi_signal_frame"); line from the test (to get it to compile), the generated assembly doesn't contain .cfi_startproc and .cfi_endproc.

However, manually adding the -g flag helps, and those directives are generated. But the -g flag is not enabled for libunwind tests and I wonder if it should be. If it shouldn't, how can we force those directives to be generated?

[miyuki]

I don't know signal frame but I think the purpose of that assert is to check unw_step return value > 0 from main if that is a signal frame. Adding another frame will make that assert always be true, regardless of signal frame, thus it is not testing signal frame anymore.

That might not be the case. unw_is_signal_frame works after the stepping out of the signal frame, i.e. we need to call unw_step anyway. @saugustine is the author of the test and he approved my patch, so presumably the test should not depend on whether the signal_frame function is main() or not.

It produces the error. And indeed, if I remove the asm(".cfi_signal_frame"); line from the test (to get it to compile), the generated assembly doesn't contain .cfi_startproc and .cfi_endproc.

I tested my patch with a baremetal compiler which adds .cfi-s to each function by default, so I didn't know that it would fail in hosted environments. I suggest to disable the test for ARM EHABI, because it does not test anything useful on the EHABI platforms anyway. Unfortunately it turned out that adding an "// unsupported:" lit comment is not trivial because the ABI is not one of CMake settings, so it's not available in lit. IMHO, the easiest way to go is to #ifdef-out the whole test() function body.

[broadwaylamb]

Unfortunately it turned out that adding an "// unsupported:" lit comment is not trivial because the ABI is not one of CMake settings, so it's not available in lit.

Coincidentally, I've added the needed CMake setting as part of solving another problem in D70815, it allowed to mark the test as unsupported.

[saugustine]

In D70397#1762812, @miyuki wrote:

I don't know signal frame but I think the purpose of that assert is to check unw_step return value > 0 from main if that is a signal frame. Adding another frame will make that assert always be true, regardless of signal frame, thus it is not testing signal frame anymore.

That might not be the case. unw_is_signal_frame works after the stepping out of the signal frame, i.e. we need to call unw_step anyway. @saugustine is the author of the test and he approved my patch, so presumably the test should not depend on whether the signal_frame function is main() or not.

Right. the first unw_step gets the context out of unw_getcontext's frame and into the current one. So having it out of main shouldn't matter.

It produces the error. And indeed, if I remove the asm(".cfi_signal_frame"); line from the test (to get it to compile), the generated assembly doesn't contain .cfi_startproc and .cfi_endproc.

I tested my patch with a baremetal compiler which adds .cfi-s to each function by default, so I didn't know that it would fail in hosted environments. I suggest to disable the test for ARM EHABI, because it does not test anything useful on the EHABI platforms anyway. Unfortunately it turned out that adding an "// unsupported:" lit comment is not trivial because the ABI is not one of CMake settings, so it's not available in lit. IMHO, the easiest way to go is to #ifdef-out the whole test() function body.

I agree that this test makes little sense for non dwarf targets, but I have no idea on the mechanics of disabling that.

[broadwaylamb]

I agree that this test makes little sense for non dwarf targets, but I have no idea on the mechanics of disabling that.

I've disabled it in D70815, could you please take a look at it?

libunwind/test/signal_frame.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Ensure that functions marked as signal frames are reported as such.
 
 #include <assert.h>
 #include <stdlib.h>
 #include <libunwind.h>
 
-int main(void) {
+void test() {
   asm(".cfi_signal_frame");
   unw_cursor_t cursor;
   unw_context_t uc;
   unw_getcontext(&uc);
   unw_init_local(&cursor, &uc);
   assert(unw_step(&cursor) > 0);
+#if !defined(_LIBUNWIND_ARM_EHABI)
   assert(unw_is_signal_frame(&cursor));
+#endif
+}
+
+int main() {
+  test();
   return 0;
 }