My first Erlang patch
July 20, 2016
Example of how you can contribute to Erlang by fixing a bug and having the fix committed to the Erlang source tree.
Finding the bug.
I’ve written a wrapper around Erlang’s erl_tidy module that reads stdin and writes the formatted result to stdout. While learning Go, I got used to gofmt and wanted the same tool for Erlang. When I used my erlfmt utility on some Erlang code I was writing, it failed.
I cut the input down until I found a small input that would trigger the crash:
$ cat t.erl
-module(t).
-export([start/0]).
start() -> io:put_chars(\"~\").
$
Fixing the bug.
The error was badarg on line 299.
** exception exit: badarg
in function erl_tidy:file/2 (erl_tidy.erl, line 299)
What is on line 299?
Line 299 in erl_tidy.erl exits and outputs the error reason returned by the child process.
$ (cd $HOME/src ; git clone https://github.com/erlang/otp.git)
$ vi \$(find $HOME/src/otp -name erl_tidy.erl)
…
292 file(Name, Opts) ->
293 Parent = self(),
294 Child = spawn_link(fun () -> file_1(Parent, Name, Opts) end),
295 receive
296 {Child, ok} ->
297 ok;
298 {Child, {error, Reason}} ->
299 exit(Reason)
300 end.
Copy erl_tidy module locally and verify same error.
I copied it locally so I could edit the file …
$ cp \$(find $HOME/src/otp -name erl_tidy.erl) .
$ cp erl_tidy.erl erl_tidy.orig
… and started by verifying that my local copy gave the same error.
$ erl
Erlang/OTP 19 [erts-8.0.1] [source-ca40008] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V8.0.1 (abort with ^G)
1> c(erl_tidy).
{ok,erl_tidy}
2> erl_tidy:file(\"t.erl\", [{stdout,true}]).
** exception exit: badarg
in function erl_tidy:file/2 (erl_tidy.erl, line 299)
3>
Figure out which line of client code is generating the error.
Since the error is passed back from a spawned process to the parent process, we don’t know the line of client code that has the problem. So, I sprinkled some io:put_char() statements to figure out where it crashes.
I knew it was related to the {stdout,true} argument since I could tidy the file without error if I did not use that option. That helped me narrow down where to put the debug statements.
$ vi erl_tidy.erl
…
$ diff -uw erl_tidy.orig erl_tidy.erl | grep -n ^
1:— erl_tidy.orig 2016-07-16 09:11:41.000000000 -0400
2:+++ erl_tidy.erl 2016-07-16 11:14:58.000000000 -0400
3:@@ -300,6 +300,7 @@
4: end.
5:
6: file_1(Parent, Name, Opts) ->
7:+ io:put_chars(\"MKB: enter file_1\n\"),
8: try file_2(Name, Opts) of
9: _ ->
10: Parent ! {self(), ok}
11:@@ -311,6 +312,7 @@
12: end.
13:
14: file_2(Name, Opts) ->
15:+ io:put_chars(\"MKB: enter file_2\n\"),
16: Opts1 = Opts ++ file__defaults(),
17: Forms = read_module(Name, Opts1),
18: Comments = erl_comment_scan:file(Name),
19:@@ -417,7 +419,9 @@
20: end.
21:
22: print_module(Tree, Opts) ->
23:+ io:put_chars(\"MKB: print_module - 1\n\"),
24: Printer = proplists:get_value(printer, Opts),
25:+ io:put_chars(\"MKB: print_module - 2\n\"),
26: io:format(Printer(Tree, Opts)).
27:
28: output(FD, Printer, Tree, Opts) ->
Run the instrumented code to see how far it gets.
$ erl
Erlang/OTP 19 [erts-8.0.1] [source-ca40008] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V8.0.1 (abort with ^G)
1> c(erl_tidy).
{ok,erl_tidy}
2> erl_tidy:file(\"t.erl\", [{stdout,true}]).
MKB: enter file_1
MKB: enter file_2
MKB: print_module - 1
MKB: print_module - 2
** exception exit: badarg
in function erl_tidy:file/2 (erl_tidy.erl, line 299)
3>
The program outputs 'MKB: print_module - 2' and then raises the error, so the next line is the culprit:
26: io:format(Printer(Tree, Opts)).
Is it io:format() or Printer()?
There are two calls on that line, so unwind them and insert another io:put_chars: Line numbers here are of erl_tidy.
421 print_module(Tree, Opts) ->
422 io:put_chars(\"MKB: print_module - 1\n\"),
423 Printer = proplists:get_value(printer, Opts),
424 io:put_chars(\"MKB: print_module - 2\n\"),
425 Text = Printer(Tree, Opts),
426 io:format(\"MKB: print_module - 3: Text = ~p\n\", [Text]),
427 io:format(Text).
When I run this, the output changes: lines 425 and 426 execute successfully, so the problem must be on line 427 in the io:format() call.
$ erl
Erlang/OTP 19 [erts-8.0.1] [source-ca40008] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V8.0.1 (abort with ^G)
1> c(erl_tidy).
{ok,erl_tidy}
2> erl_tidy:file(\"t.erl\", [{stdout,true}]).
MKB: enter file_1
MKB: enter file_2
MKB: print_module - 1
MKB: print_module - 2
MKB: print_module - 3: Text = \"-module(t).\n\n-export([start/0]).\n\nstart() -> io:put_chars(\\"~\\").\"
** exception exit: badarg
in function erl_tidy:file/2 (erl_tidy.erl, line 299)
3>
The fix.
After some staring at the 'print_module - 3' line above, the problem is clear. The tilde character is the format identifier character in Erlang, as the percent character is in C.
The problem can be duplicated most concisely in the console like this:
4> io:format(\"~\").
** exception error: bad argument
in function io:format/3
called as io:format(<0.50.0>,\"~\",[])
5>
Since the intent is to print a string, changing io:format() to io:put_chars() fixes the bug.
Committing the fix to the Erlang source tree.
Erlang uses the normal github process for pull requests.
The Erlang team provides a well-written set of instructions on how to submit a patch.
It’s pretty much the normal github flow, with a couple wrinkles:
- use git config to setup your name and email
- for bug fixes, patch the maint branch
- test first
- write a good commit message
- clean up your patch before submitting pull request
Building Erlang and running the syntax_tools test suite.
Again, great docs: INSTALL.md and TESTING.md.
$ git clone git@github.com:mbucc/otp.git
$ cd otp
$ export ERL_TOP=‘pwd‘
$ ./otp_build autoconf # (6s)
$ ./otp_build setup -a # (11m)
$ export PATH=‘pwd‘/bin:$PATH
$ ./otp_build tests # (1m)
$ cd lib/syntax_tools/
$ ct_run -pa ../../release/tests/test_server/ -suite ./test/syntax_tools_SUITE
Erlang/OTP 19 [erts-8.0.1] [source-ca40008] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]
Converting \"../../release/tests/test_server\" to \"/Users/mark/src/otp/release/tests/test_server\" and re-inserting with add_patha/1
Common Test starting (cwd is /Users/mark/src/otp/lib/syntax_tools)
Eshell V8.0.1 (abort with ^G)
(ct@marks-mbp-3)1>
Common Test: Running make in test directories…
Recompile: syntax_tools_SUITE
CWD set to: \"/Users/mark/src/otp/lib/syntax_tools/ct_run.ct@marks-mbp-3.2016-07-19_07.24.34\"
TEST INFO: 1 test(s), 11 case(s) in 1 suite(s)
Testing lib.syntax_tools.syntax_tools_SUITE: Starting test, 11 test cases
Testing lib.syntax_tools.syntax_tools_SUITE: TEST COMPLETE, 11 ok, 0 failed of 11 test cases
Updating /Users/mark/src/otp/lib/syntax_tools/index.html … done
Updating /Users/mark/src/otp/lib/syntax_tools/all_runs.html … done
$
However, this is not the way they tell you to do it!
The TESTING.md instructions say to use the ts() module. However, that approach does not notice and recompile changed files. The ct_run approach above does.
If you do use the ts() module, and you are on OSX, use the 'batch' option so XQuartz doesn’t open; for example,
1> ts:run(syntax_tools, syntax_tools_SUITE, [batch]).
or
1> ts:run(syntax_tools, syntax_tools_SUITE, t_erl_tidy, [verbose, batch]).
Final notes
The pull request is https://github.com/erlang/otp/pull/1071 and the resulting commit is https://github.com/erlang/otp/commit/0969ecad9e2978bacb7b27f663cd16aa5ea6c460. Note that in this case, I was too slow with the git squash (aka rebase) and Hans Bolinder did it for me.
It took less that a month to get committed, and that was delayed because the Erlang team was in the middle of a release.