Update framepointers tests to avoid false positive with inlined C functions #11594

fabbing · 2022-10-03T13:37:38Z

As explained correctly in 11582 we don't have much control over the backtrace look with C functions: gcc can choose to inline or not some of them.
We probably still want to have minimal checks beyond caml_program (the starting point where our framepointers must be checked) while avoiding false positive on inlined function.
This PR proposes adding an awk script to validate functions from main..caml_start_program are the expected ones and in correct order but discard them, so .reference contains only the least likely to change.
This PR stops the frame-pointers backtrace at caml_program to ignore the differences in case of a different inlining decision by the C compiler.
It also does all of the previous "backtrace post-processing" in situ of the stack-walking-with-frame-pointers C code. So awk is not needed, and the previous dependency on sed or sh are also completely removed.

dra27 · 2022-10-03T16:10:25Z

I like the idea, but I really don't think we should introduce awk into the testsuite runners - although it's mitigated by the fact that frame-pointers mode is Linux-only (I think??), it's still a portability nightmare.

My instinct on this is to move the processing of the stack trace from the shell script back into ocamltest - i.e. have the test programs emit a fairly raw backtrace as now, but instead of using a run hook to replace native with a new action. The string processing is not particularly onerous (and could use ocamllex, if really needed). @shindere, what do you think?

Equally, are we straying into over-thinking all this (@xavierleroy, what do you think??)

fabbing · 2022-10-03T16:22:41Z

Yes, at the moment, frame-pointers support is only for amd64 on Linux (it should however work on other Unixes).

xavierleroy · 2022-10-04T16:26:26Z

Equally, are we straying into over-thinking all this

Quite possibly :-) My general impressions:

Some tests are very useful to get the implementation right, yet quite hard to turn into non-regression tests afterwards. A typical example is testing for fairness of scheduling. Maybe this is one of those tests. In which case we just keep them around but don't mark them as ocamltest tests.
Mixing sh, sed and awk in the same script is getting ugly, not even mentioning the portability problems @dra27 alludes to. I'm pretty sure all the filtering can be done in pure sed, or in pure sh, but the code wouldn't be pretty.
I still wish we'd standardize on Python for all the scripts of the OCaml distribution. It's not a great language but at least all the kids learn it in school these days, so we'll have maintainers when we retire.
No way you'd change ocamltest to handle the postprocessing of the trace! If you want to do your postprocessing in OCaml, you can write it in OCaml, have it compiled by ocamltest, and call it from the run script. Leave ocamltest as generic as possible, please.
The C code in fp_backtrace.c could also filter / sanitize / check the validity of the trace itself before printing it, or not print the trace but produce an OCaml list of function names which would then be filtered / sanitized / checked by OCaml code.

shindere · 2022-10-09T20:27:22Z

Xavier Leroy (2022/10/04 09:26 -0700):

- I still wish we'd standardize on Python for all the scripts of the OCaml distribution. It's not a great language but at least all the kids learn it in school these days, so we'll have maintainers when we retire.

Would you use Python even for the scripts required to build the compiler? In other words, would it be okay to have Python as a build dependency? In my understanding there are two reaons why we didn't do it so far. First, it is reported to be easy to write scripts that are not portable from one version of Python to another. I don't know to which extent this is true. Second, I have the idea that the Python requirement is not as easily satisfied on Windows that it is on Linux, that's why there is resistance to depending on it. What do you think about these reasons, @xavierleroy? Especially about the second one?

- No way you'd change `ocamltest` to handle the postprocessing of the - trace! If you want to do your postprocessing in OCaml, you can write it in OCaml, have it compiled by `ocamltest`, and call it from the `run` script. Leave `ocamltest` as generic as possible, please.

Yes, indeed. So far, the perhaps-not-explained-enough rule was that we build something into ocamltest iff it's needed by more than one test.

xavierleroy · 2022-10-11T08:38:36Z

This is off-topic for this PR, but:

First, it is reported to be easy to write scripts that are not portable from one version of Python to another. I don't know to which extent this is true.

This can't be worse than our current gawk vs mawk 1.3.3 vs mawk 1.3.4 situation, not to mention the odd non-GNU sed.

Second, I have the idea that the Python requirement is not as easily satisfied on Windows that it is on Linux, that's why there is
resistance to depending on it.

I see Python in the Microsoft Store and as Cygwin packages. Nobody bothered to explain the Windows issues yet; I'm all ears.

nojb · 2022-10-11T08:44:29Z

I see Python in the Microsoft Store and as Cygwin packages. Nobody bothered to explain the Windows issues yet; I'm all ears.

No issues that I am aware of. It is easy to install under Cygwin in any case.

dra27 · 2022-10-11T10:24:55Z

I'm not sure whether this is focussed at our "infrastructure" (testsuite, as here, support scripts like tools/check-typo, tools/pre-commit-githook, etc.) or the "build system" (i.e. what's needed for make world.opt). On the build system side, I personally don't think it's at all good that we switch to having opam switch create foo ocaml.5.something.0 require a working Python installation (on any system) - but that's no reflection of Python or Windows, it's just on adding a not-installed-by-default-everywhere dependency. I also FWIW have "glaciated" work from last year on the build system side that gets rid of all of the relatively tiny uses of sed and awk in the build system.

For the infrastructure, I personally anticipate that we swap one set of papercuts for another, but I don't have a strong objection. I think it is good, for example, that the installation instructions for tools/pre-commit-githook involve copying it to the correct place in your OCaml clone and it then just works (including with Git-for-Windows) and that becomes fractionally more complicated if it's written in Python. Similarly, I think it's good that if you've managed to build OCaml, make -C testsuite all should also just work and we'd potentially lose that, too. But all of that only affects the contribution process to OCaml - that should of course be as easy as possible, but I think it's axiomatically less important than the user experience of OCaml.

For the actual Windows papercuts which may or may not happen. We've already mentioned two entirely different distributions of Python, both assuming different file system conventions. It's that kind of thing - I would anticipate that where occasionally now we hit a funny locale, an overly-stringent POSIX awk, we'll start hitting an unrecognised path format because the user provided us with a Python from something we (and possibly they) hadn't thought of. It doesn't mean it'd be a bad idea to switch to Python, but if we're going to switch to something, I'd prefer it to be utopia, that's all! Given that it's often been me doing it, I kinda prefer chasing down a GNU-ism on my aged Macbook, with the POSIX spec in the other hand, to trying to trace exactly which executable has invoked what in a cryptic CI run and why the Windows file path hasn't been accepted by it (which I have to do quite a lot where git is concerned in the opam project, where we hit exactly this kind of problem and annoyingly regularly).

Another attempt to ignore potentially inlined C functions while still checking reasonable behavior

In frame-pointers tests, the backtrace post-processing is performed directly in fp_backtrace, and so, no longer depends on filter-locations.sh (that was using sed and awk scripts).

fabbing · 2023-03-10T14:57:35Z

The last commit moves all the backtrace post-processing completely into fp_backtrace.c, so filter-locations.sh is no longer needed, nor the awk and sed scripts it was using.

Hopefully this commit also passes the INRIA CI with more exotic configurations!

fabbing · 2023-03-15T16:29:45Z

It looks fine on the Inria CI as well!
This PR is again ready for review.

shindere · 2023-07-17T15:42:08Z

I'm sorry but I can't review this PR myself.

Has anything been decided?

gasche · 2023-09-20T09:45:55Z

In #12588 I wrote logic in C to stop the printed backtrace at caml_startup | caml_main. This is an alternative approach to the present awk-script suggestion that could prove more consensual.

shindere · 2023-09-20T10:36:04Z

Did you actually have a look at the code of the present PR? Because AFAIUI this PR does get rid of scripts by doing everything in C already.

gasche · 2023-09-20T11:36:57Z

No I didn't, I had forgotten that the present PR existed.

Because AFAIUI this PR does get rid of scripts by doing everything in C
already.

Indeed. On the other hand, the change to the C code in the present PR adds 117 lines and removes 71, while my PR adds 22 lines and removes 6. Maybe we can still hope for faster review.

fabbing · 2023-09-20T13:12:19Z

This PR has evolved quite a bit from the very first implementation, to try and account for commenters opinion, especially on the delicate topic of awk, sed and scripts portability.
My bad for not updating the PR description accordingly.

Indeed. On the other hand, the change to the C code in the present PR adds 117 lines and removes 71, while my PR adds 22 lines and removes 6. Maybe we can still hope for faster review.

The bigger diff comes from that the all backtrace processing is now done in pure C, with POSIX regex, so it's obviously more C code but also removes the need for any of the sh scripts and sed calls.

testsuite/tests/frame-pointers/fp_backtrace.c

Octachron · 2023-10-04T12:39:50Z

testsuite/tests/frame-pointers/fp_backtrace.c

-
-  if (!sigsetjmp(resume_buf, 1)) {
-    *prev = fi->prev;
-    *retaddr = fi->retaddr;


Do I read correctly, that the signal mask and long jump business in safe_read was here to handle the read to retaddr, and this is why the present PR remove the function (and it auxiliary functions)?

Yes, this is absolutely correct.
Previously, fp_backtrace tried to unwind the whole stack, but the linked list of 'saved rbp' was not always NULL terminated, as I expected. safe_read installed a temporary signal handler to gracefully handle an invalid pointer deference attempt.

Stopping the backtrace (and thus stack unwinding) at caml_program means this dubious trick is no longer needed.

Octachron

Comparing #12588 and this PR, I feel that #12588 is the simpler fix, whereas this PR ends up with the simpler code:

all processing is in one place and one language
the regexp matching code is as complex as before
removing signal handling and longjumps from the test code feels like a clear win.

Now that I have reviewed both PRs, I am in favor of merging this version.

Octachron · 2023-10-05T09:03:28Z

testsuite/tests/frame-pointers/fp_backtrace.c

-  return 0;
+  //regoff_t len = funcname->rm_eo - funcname->rm_so;
+  //return strnstr(symbol + funcname->rm_so, CAML_ENTRY, len) == 0;
+  return strstr(symbol + funcname->rm_so, CAML_ENTRY) != NULL;


This test seems slightly too lax, since we are testing that symbol at offset funcname contains CAML_ENTRY somewhere.

That's right!
I don't know why I stopped using strncmp like I did as in is_from_executable... Fixed in 3dc39fe.

Octachron · 2023-10-06T08:06:05Z

Squashed and merged, thanks for the simplification and sorry for the late review of the final version !

…ctions (ocaml#11594) Another attempt to ignore potentially inlined C functions while still checking reasonable behavior In frame-pointers tests, the backtrace post-processing is performed directly in fp_backtrace, and so, no longer depends on filter-locations.sh (that was using sed and awk scripts).

dra27 added the no-change-entry-needed label Oct 3, 2022

fabbing added 2 commits March 10, 2023 14:47

Update framepointers tests to avoid false positive

b1a416d

Another attempt to ignore potentially inlined C functions while still checking reasonable behavior

Handle backtrace post-processing in fp_backtrace

9a7aeff

In frame-pointers tests, the backtrace post-processing is performed directly in fp_backtrace, and so, no longer depends on filter-locations.sh (that was using sed and awk scripts).

fabbing force-pushed the fp-tests-inline-fix branch from 64ad832 to 9a7aeff Compare March 10, 2023 14:06

Fix accidental revert of typo fix in fp/c_call.ml

926c378

fabbing force-pushed the fp-tests-inline-fix branch from bc7d6b3 to 926c378 Compare March 15, 2023 16:19

fabbing mentioned this pull request Jun 1, 2023

Add ThreadSanitizer support #12114

Merged

5 tasks

fabbing mentioned this pull request Sep 20, 2023

Test failure when glibc has frame pointers #12587

Closed

gasche mentioned this pull request Sep 20, 2023

Frame pointer tests: stop backtraces at caml_main/caml_startup #12588

Merged

damiendoligez self-assigned this Sep 20, 2023

Octachron reviewed Oct 4, 2023

View reviewed changes

testsuite/tests/frame-pointers/fp_backtrace.c Outdated Show resolved Hide resolved

Octachron reviewed Oct 4, 2023

View reviewed changes

Remove comment of alternative code

9e2ef94

Octachron approved these changes Oct 5, 2023

View reviewed changes

Proper symbol comparison with CAML_ENTRY

3dc39fe

Octachron merged commit 6a0a0a4 into ocaml:trunk Oct 6, 2023
9 checks passed

gasche mentioned this pull request Oct 7, 2023

generalize tests/unwind to work on clang -O0 #12437

Merged

jamesjer mentioned this pull request Dec 15, 2023

OCaml 5.1.x: out of range branch on s390x #12829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update framepointers tests to avoid false positive with inlined C functions #11594

Update framepointers tests to avoid false positive with inlined C functions #11594

fabbing commented Oct 3, 2022 •

edited

Loading

dra27 commented Oct 3, 2022

fabbing commented Oct 3, 2022

xavierleroy commented Oct 4, 2022

shindere commented Oct 9, 2022 via email

xavierleroy commented Oct 11, 2022

nojb commented Oct 11, 2022

dra27 commented Oct 11, 2022

fabbing commented Mar 10, 2023

fabbing commented Mar 15, 2023

shindere commented Jul 17, 2023

gasche commented Sep 20, 2023

shindere commented Sep 20, 2023 via email

gasche commented Sep 20, 2023

fabbing commented Sep 20, 2023

Octachron Oct 4, 2023

fabbing Oct 4, 2023

Octachron left a comment

Octachron Oct 5, 2023

fabbing Oct 5, 2023

Octachron commented Oct 6, 2023

Update framepointers tests to avoid false positive with inlined C functions #11594

Update framepointers tests to avoid false positive with inlined C functions #11594

Conversation

fabbing commented Oct 3, 2022 • edited Loading

dra27 commented Oct 3, 2022

fabbing commented Oct 3, 2022

xavierleroy commented Oct 4, 2022

shindere commented Oct 9, 2022 via email

xavierleroy commented Oct 11, 2022

nojb commented Oct 11, 2022

dra27 commented Oct 11, 2022

fabbing commented Mar 10, 2023

fabbing commented Mar 15, 2023

shindere commented Jul 17, 2023

gasche commented Sep 20, 2023

shindere commented Sep 20, 2023 via email

gasche commented Sep 20, 2023

fabbing commented Sep 20, 2023

Octachron Oct 4, 2023

Choose a reason for hiding this comment

fabbing Oct 4, 2023

Choose a reason for hiding this comment

Octachron left a comment

Choose a reason for hiding this comment

Octachron Oct 5, 2023

Choose a reason for hiding this comment

fabbing Oct 5, 2023

Choose a reason for hiding this comment

Octachron commented Oct 6, 2023

fabbing commented Oct 3, 2022 •

edited

Loading