[libcxx-commits] [PATCH] D70397: [libunwind] Adjust the signal_frame test for Arm

Mon Dec 2 09:49:51 PST 2019

saugustine added a comment.

In D70397#1762812 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70397/new/

https://reviews.llvm.org/D70397