Comment author: cullmann

The lines quoted above got introduced for

#6031

Comment author: @mshinwell

I think you do need 16-byte alignment here. The problem is probably that Clang has generated some instruction that faults if an operand isn't 16-byte aligned.

Comment author: cullmann

Perhaps other places modified with that #ifdef needs review, too. Given problems occur only for some instruction generations in the C compiler that is a hard to track thingy.

Comment author: @gasche

We tried to use OCaml together with the $with_frame_pointers option to have usable perf profiling.

You don't need frame pointers for usable perf profiling, you can use the DWARF information produced by (ocamlopt -g) with the --call-graph=dwarf option:

https://ocaml.org/learn/tutorials/performance_and_profiling.html#UsingperfonLinux

(Using perf on binaries without -g will give you per-function costs, but not aggregated by call stacks.)

Comment author: cullmann

You don't need frame pointers for usable perf profiling, you can use the DWARF information produced by (ocamlopt -g) with the --call-graph=dwarf option:

We tried that, like for C/C++ the resulting stack frame chains are much more imprecise and more often broken for large things.

Comment author: @mshinwell

You also need a fairly recent version of perf (and kernel maybe?) for DWARF/perf profiling.

Comment author: cullmann

You also need a fairly recent version of perf (and kernel maybe?) for DWARF/perf profiling.
I have both, that doesn't matter.
You just miss more frames as it is much more expensive the use the DWARF variant.
Anyways, I think that is offtopic, the with-frame-pointer option must still work OK and not generate code that breaks with compilers assuming 16 byte alignment (if that is the case here).

Comment author: @mshinwell

I will investigate making a patch for this issue, and see if I can spot any other cases where the alignment is wrong.

Comment author: cullmann

I would propose a patch like:

--- a/src/asmrun/amd64.S
+++ b/src/asmrun/amd64.S
@@ -438,12 +438,15 @@ LBL(103):
CFI_ADJUST(8)
RECORD_STACK_FRAME(8)
#ifdef WITH_FRAME_POINTERS

•    /* Do we need 16-byte alignment here ? */
  

•    /* ensure 16 byte alignment by subq + enter using 16-bytes, #7417 */
  

•    subq    $8, %rsp; CFI_ADJUST (8)
     ENTER_FUNCTION
  

#endif
call LBL(caml_call_gc)
#ifdef WITH_FRAME_POINTERS

•    /* ensure 16 byte alignment by leave + addq using 16-bytes #7417 */
     LEAVE_FUNCTION
  

•    addq    $8, %rsp; CFI_ADJUST (-8)
  

#endif
popq %rax; CFI_ADJUST(-8) /* recover desired size */
jmp LBL(caml_allocN)

I inspected the other #ifdef'd locations, they look not wrong to me.

Comment author: cullmann

Created small pull request for that change:

#930

Comment author: cullmann

I can now confirm that all company internal regression tests work with that again (e.g. clang + -with-frame-pointers does again the same as without the option)

Comment author: @xavierleroy

Patch merged on trunk, commit 3ad607f. OK for closing? or should it also go on 4.04 release branch?

Comment author: cullmann

If there is some 4.04.x patch release, I think it would make sense to have it in the branch for that.

Comment author: @gasche

I understand Damien's initial triaging as an indication of support for a 4.0x fix, so I'll also cherry-pick in 4.04. Thanks to Xavier for the triaging and being careful about that.

Comment author: @gasche

Merged in the 4.04 release branch as a10770e. Thanks!