Page MenuHomePhabricator

[libunwind] Adjust the signal_frame test for Arm
ClosedPublic

Authored by miyuki on Nov 18 2019, 7:22 AM.

Details

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.

Diff Detail

Event Timeline

miyuki created this revision.Nov 18 2019, 7:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
saugustine accepted this revision.Nov 18 2019, 1:39 PM
This revision is now accepted and ready to land.Nov 18 2019, 1:39 PM
This revision was automatically updated to reflect the committed changes.
broadwaylamb added a subscriber: broadwaylamb.EditedNov 20 2019, 6:08 AM

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 added a subscriber: steven_wu.EditedNov 20 2019, 9:45 AM

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);.

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()?

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 added a comment.EditedNov 28 2019, 2:56 AM

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?

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.

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.

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 added a comment.EditedDec 2 2019, 9:52 AM

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?