symbol names: keep using . on non-Apple unix systems
#14143
Conversation
| @@ -7,4 +7,4 @@ ${program} | |||
|
|
|||
| # now check the assembly file produced during compilation | |||
| asm=${test_build_directory}/func_sections.s | |||
| grep "\.section \.text\.caml\.camlFunc_sections\\$" "$asm" | wc -l | tr -d ' ' | sed '/^$/d' | |||
| grep "\.section \.text\.caml\.camlFunc_sections." "$asm" | wc -l | tr -d ' ' | sed '/^$/d' | |||
There was a problem hiding this comment.
Using just . matches any character. I'm not sure this is what you had in mind. Maybe something like that instead?
| grep "\.section \.text\.caml\.camlFunc_sections." "$asm" | wc -l | tr -d ' ' | sed '/^$/d' | |
| grep "\.section \.text\.caml\.camlFunc_sections[.$]" "$asm" | wc -l | tr -d ' ' | sed '/^$/d' |
There was a problem hiding this comment.
That was a quick fix, but yes I agree it is better to narrow this grep.
Changes
Outdated
| * #13050: Use '$' instead of '.' to separate module names in symbol names. | ||
| This changes mangling of OCaml identifiers from | ||
| * #13050, #14104, #?????: Use '$' instead of '.' to separate module names | ||
| in symbol names on Apple and non-unix operating systems. |
There was a problem hiding this comment.
I think being more explicit about which platforms are affected would be a bit nicer:
| in symbol names on Apple and non-unix operating systems. | |
| in symbol names on macOS and Windows (including the Cygwin backend). |
There was a problem hiding this comment.
It is also slightly more correct since with the current code any new operating system will end up using . as a symbol name separator.
| (lldb) create camlMeander[.$]c_to_ocaml* | ||
| Breakpoint created for regex camlMeander[.$]c_to_ocaml*. |
There was a problem hiding this comment.
This change looks wrong. Keep using \$ ?
There was a problem hiding this comment.
Using the [.$] regexp allows to keep the same lldb script on all platforms. Otherwise, we would need to have a lldb-linux-script and a lldb-script. The quirky regexp seemed better to me than the duplication.
There was a problem hiding this comment.
Hmm... the file name says it's a reference file and it is for macos. There is also a linux-lldb-arm64.reference in the same directory. Not sure how to reconcile this with your reply.
There was a problem hiding this comment.
The tests are sharing the same debugger script:
debugger_script = "${test_source_directory}/lldb-script";and it is this script which is echoed inside the reference file.
| (lldb) create camlMeander[.$]c_to_ocaml* | ||
| Breakpoint created for regex camlMeander[.$]c_to_ocaml*. |
| "camlCmm.5": | ||
| addr "camlCmm.standard_atomic_get_274" |
There was a problem hiding this comment.
We need to make sure that this test is not run on macOS and Windows. For the record: I thoroughly dislike tests that match on dumps of internal compiler passes.
There was a problem hiding this comment.
The test is even more restricted than that: it is restricted to linux, amd64, no flambda and no-tsan .
| @@ -56,6 +56,9 @@ val current_unit_symbol: unit -> Symbol.t | |||
| val symbol_separator: char | |||
| (* Return the module separator used when building symbol names. *) | |||
|
|
|||
| val escape_prefix: string | |||
| (* Return the escape prefix for hexadecimal escape sequence. *) | |||
There was a problem hiding this comment.
Perhaps, to be extra clear:
| (* Return the escape prefix for hexadecimal escape sequence. *) | |
| (* Return the escape prefix for hexadecimal escape sequences in symbol names. *) |
As discussed in #14104, this PR proposes to revert to using
.as module separator for symbol names on non-Apple unix systems — at the very least for OCaml 5.4.This should restore the support of OCaml symbol name mangling in perf for OCaml 5.4, while keeping the lldb fix on Apple systems . Moreover, it seems likely to me that any tools analyzing symbol names already needs to have OS specific paths, thus we are hopefully not exporting a noticeable amount of complexity to those external tools.
cc @tmcgilchrist