Enable frame pointers on macOS x86_64

[tmcgilchrist]

Enables frame pointers on macOS x86_64 reusing the existing code for Linux x86_64 frame pointers.

[gasche]

@fabbing would you by chance be available to have a look at the PR?

[fabbing]

Yes, I have already started looking into this.
It looks fine, but I still need to convince myself that the removal of is_from_executable is also correct.

[fabbing]

I'm pretty convinced now that it should be fine!

[shindere]

This looks good to me, thanks.

Can you please add Fabrice as a reviewer in the Changes entry?

[gasche]

Out of curiosity, can someone explain why the removal of is_from_executable is fine? (I can make a guess but it helps to have this reasoning about the code written out loud, it helps build a shared body of knowledge about the codebase.)

[tmcgilchrist]

The original intent for that code was to filter out parts of the backtrace that were not from the executable under test. I noticed when modifying the regex to support macOS symbols that it was not hitting that code and so wasn't excluding any frames from the backtrace. Running it again on Linux without is_from_executable was also fine.

I guess that @fabbing was originally running these tests under gdb during development and was getting elements of the backtrace from there.

[shindere]

@gasche: if you think you should have been added as a reviewer, do not
hesitate to either push a commit directly or to ask me to do so. I was
unsure but wanted to move on.

[gasche]

No need, I did not do a full review of the change. Thanks!

[shindere]

Gabriel Scherer (2024/05/16 00:48 -0700):

No need, I did not do a full review of the change. Thanks!

Okay, no problem, just wanted to offer theopportunity.

[fabbing]

Out of curiosity, can someone explain why the removal of is_from_executable is fine? (I can make a guess but it helps to have this reasoning about the code written out loud, it helps build a shared body of knowledge about the codebase.)

This is_from_executable function was introduced by 6a0a0a4, as a way to ignore external libraries and to stop frame-pointer based unwinding to unwind further than the main function. On some systems, it was actually possible to unwind functions up to the dynamic loader.
If this test is removed, and we were to cross into another binary, the regex will probably fail and filter it. If the function name would still match the regex, the reference will flag anyway the unexpected function.

And as I writing this, I realised that we should also have updated the OCaml side of the code to discard this no longer required argument, and the left over in the C code (argv0 and execname). Should I open a new PR for this?

[shindere]

Thanks a lot for your very clear explanation @fabbing, which makes perfect sense to me. Fabrice (2024/05/21 06:23 -0700):

Should I open a new PR about this?

I don't think you have to if you don't feel like doing so, but I also thinkit will be very welcome if you do! :)