-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix stack overflow on large string constants #1562
Conversation
test/passing/dune.inc
Outdated
(deps .ocamlformat ) | ||
(action | ||
(with-outputs-to large_string.ml.output | ||
(run %{bin:ocamlformat} --profile=janestreet %{dep:large_string.ml})))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how long does this test take to run? also I'm wondering if it makes sense to generate the string instead of storing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost instant for me, and I'm running on a slow vm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of generating this test, either as a dune rule or as a cram test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to be difficult to generate a string that is already formatted (-n 1), maybe that would be enough to have a unit test for list_pn instead of testing the absence of exception of the ml file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha yes I didn't realize the file was already format. let's scratch that idea and keep the huge file. the unit test is complementary for that, it won't check the resources, just the result. so it all seems good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can reproduce it with echo "let _ = \"$(printf '%*s' 300000 | sed 's/ /_ _/g')\"" | ocamlformat - --impl -o /dev/null --debug
but I don't get any trace
my bad that wasn't the bytecode one, the backtrace is made of countless lines of
Called from Ocamlformat_lib__Fmt.T.($).(fun) in file "lib/Fmt.ml", line 50, characters 27-30
ending with another stack overflow, on ( $ )
this time @emillon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'll try if this can be fixed with a different encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a fix for this, I'll open a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #1568. Then you can write:
let l =
let rec aux (prev, acc) = function
| [] -> acc
| [xI] -> aux (xI, (Some prev, xI, None) :: acc) []
| xI :: (xJ :: _ as xJN) ->
aux (xI, (Some prev, xI, Some xJ) :: acc) xJN
in
aux (x1, [(None, x1, Some x2)]) x2N
in
List.rev_map l ~f:(fun (prev, x, next) ->
lazy_ (fun () -> pp ~prev x ~next) )
|> sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I rebased this one, the CI is now ok!
I added a test in #1566. |
This is still segfaulting on ppc64 though, interesting |
1efb7d4
to
157d2d2
Compare
157d2d2
to
d5d2429
Compare
Fix #1561, no diff with test_branch.