JL_STD* vs ios_std* cleanup #9450

samoconnor · 2014-12-24T08:37:58Z

Use jl_printf(), jl_puts() etc instead of ios_printf(), JL_PRINTF(), fprintf() etc for consistency.

Make jl_printf(JL_STD*, ...), jl_puts() safe in very early stages of initialisation.

Try to remove miscellaneous ios.c dependencies.
Remaining ios.c dependencies are:

flisp/*
iostream.jl (and supporting C functions in sys.c) -- uses ios.c to do file IO.
dump.c, -- uses ios_mem() for RAM buffers and ios_file() to read/write files.

See detail in commit logs and discussion here:
https://groups.google.com/forum/?fromgroups=#!topic/julia-dev/UlfIRvX0_nw

tkelman · 2014-12-24T09:03:31Z

Missing several JL_PRINTF in src/debuginfo.cpp, see https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.823/job/je98ejyr0jv2sqsm#L311

Once it's working, seems like a good idea.

samoconnor · 2014-12-24T10:56:48Z

looks like the Travis OSX build is stuck trying to download a new version of mpfr ??

Keno · 2014-12-24T10:58:51Z

cc @staticfloat for the Travis OS X problem

tkelman · 2014-12-24T11:01:48Z

I think it's a SourceForge problem, I doubt Elliot can do anything about it other than encourage the Homebrew maintainers to switch to S3.

tkelman · 2014-12-24T11:58:21Z

Hey well aside from the OSX Travis timeout, that seemed to work, no new warnings on Windows now and it passes tests (AppVeyor doing its job nicely there).

I'm not so sure about the asserts and the fallback behavior in jl_write, but I'll let others (mostly @vtjnash) make the call.

vtjnash · 2014-12-24T14:31:47Z

src/dump.c

@@ -1306,7 +1306,7 @@ DLLEXPORT void jl_save_system_image(const char *fname)
    htable_reset(&backref_table, 250000);
    ios_t f;
    if (ios_file(&f, fname, 1, 1, 1, 1) == NULL) {
-        JL_PRINTF(JL_STDERR, "Cannot open system image file \"%s\" for writing.\n", fname);
+        jl_printf(JL_STDERR, "Cannot open system image file \"%s\" for writing.\n", fname);
        exit(1);


samoconnor · 2014-12-24T15:07:44Z

re static initializers:

man stdio says:

FILE *stdin;  FILE *stdout; FILE *stderr;
 Note: The current implementation does not allow these variables to be evaluated at C compile/link time.

vtjnash · 2014-12-24T15:24:16Z

Good catch. Use the numbers 0,1, 2 then (cast as pointers)

vtjnash · 2014-12-24T15:26:47Z

ui/repl.c

@@ -125,7 +125,7 @@ void parse_opts(int *argcp, char ***argvp)
            jl_compileropts.cpu_target = strdup(optarg);
            break;
        case 'h':
-            ios_printf(ios_stdout, "%s%s", usage, opts);
+            jl_printf(JL_STDERR, "%s%s", usage, opts);
            exit(0);


if you are calling jl_printf, then you must call jl_exit (even though we know that in this case it short-circuits and just calls fprintf)

Calling jl_exit() this early causes a segfault.
I guess there is not enough stuff initialised yet.
I'll look into making jl_exit() safe against early calls...

yeah, i wondered if it might. jl_exit assumes that you've successfully made it through julia_init (although clearly there are cases where it gets called inside of julia_init, so i'm not sure what would happen there)

samoconnor · 2014-12-24T15:29:40Z

My first implementation did use STDOUT_FILENO, ... (i.e. 0, 1, 2 & 3).
However, it seemed dangerous to overload NULL and STDOUT_FILENO.
e.g. it seemed like a good thing to be able to do assert(stream != NULL).
After that, address of FILE* stdout, seemed like as good a magic number as any...

I could always define some non-zero static constants and use them instead.

vtjnash · 2014-12-24T15:37:10Z

some non-zero static constants sounds good then. it'll help it segfault faster if someone uses the wrong function on them

samoconnor · 2014-12-25T21:31:28Z

Thanks @vtjnash for all your feedback.
What is the etiquette around here for pull requests that grow to 19 commits?
Should I re-do the pull request from a branch that has a small set of clean commits?

vtjnash · 2015-01-20T19:38:45Z

src/codegen.cpp

    if (!jl_ExecutionEngine) {
-        JL_PRINTF(JL_STDERR, "Critical error initializing llvm: ", ErrorStr.c_str());
+        jl_printf(JL_STDERR, "Critical error initializing llvm: ", ErrorStr.c_str());


jl_errorf ?

That would probably have the same effect.
However, per my note Jan 19, the time I can spend on this PR is now very limited.
I'll attempt to make things right if places are found where I've broken something.
But, I dont intend to add any more new changes on top of the existing refactoring.

samoconnor · 2015-02-02T06:28:48Z

bump - see comment Jan 19

tkelman · 2015-02-02T16:09:15Z

Which one? It went conflict-stale again like I thought it would. Will you have time to rebase this, or will @vtjnash have to adopt the branch to get it rebased for merging? He's the main person qualified to make the final decision on it.

samoconnor · 2015-02-02T23:22:16Z

rebased again...

samoconnor · 2015-02-06T09:55:24Z

rebased again for review by @vtjnash

@vtjnash

Rationale: Unravel ios.c’s legacy role as debug io library for the runtime; vs current role as backend for iostream.jl. Clean up historical debug printf functions: Replace calls to ios_printf(), ios_puts(), JL_PRINTF, JL_PUTS etc with jl_printf(), jl_puts()... Replace exit() with jl_exit() in repl.c, dump.c and init.c. Make jl_printf() and jl_exit() safe in early initialisation. Remove ios.c dependance from ast.c and builtins.c: src/flisp/flisp.c: New fl_load_system_image_str(char*, size_t) Hides detail of ios_static_buffer from ast.c ios_* no longer used in ast.c. src/builtins.c In jl_errorf(), replace ios_vprintf() with vasprintf() (following the precedent set by src/jl_uv.c:663). Use free() not LLT_FREE() to clean up after vasprintf() in jl_uv.c: man vasprintf says “asprintf() and vasprintf() dynamically allocate a new string with malloc(3)” Refactor pairs of calls to jl_printf() & jl_exit(1) into calls to jl_error[f](), which does the same thing during early init. Dead code removal: jl_uv.c: jl_bufptr(), jl_ios_buf_base(), jl_putc(), jl_pututf8() sys.c: jl_ios_size() New jl_safe_printf(). jl_safe_printf() uses a statically allocated 1000 byte buffer to vnsprintf() a message then calls write(2) to send it to stderr. jl_safe_printf() is intended to be used in places where there is the potential for out of memory, corrupted stack, re-entrancy etc. The current jl_safe_printf() implementation is not 100% safe in all situations, but it should be pretty good most of the time, especially if the formatting is limited to %d. The safety of jl_safe_printf() could be improved later, but at least for now the places were safety might matter are identified by its use. jl_safe_printf() is used in: - sigdie_handler() - sigint_handler() - _exception_handler() - catch_exception_raise() - gdblookup() Thanks to @vtjnash for assistance and feedback with these changes. Dead code cleanup & jl_write() simplificaiton. Clean up dead code: stream.jl : write!() — My apologies if this is used somewhere, I could’t find any doc or callers. builtins.c : jl_print_symbol(), jl_print_int64() ccall.cpp : if(0 …) jl_puts() jl_uv.c : jl_puts Replace 2 remaining calls to jl_puts() with calls to jl_printf() for consistency. builtins.c : jl_error() repl.c : true_main() Move jl_uv_writecb() implementation from stream.jl to jl_uv.c. This callback is only used by jl_write(), which is only called from jl_vprintf() in runtime C code. This helps to de-tangle fs.jl and stream.jl from the runtime's printf(). Use jl_safe_printf() to attempt error message output in jl_uv_writecb(). If this callback is printing an error, there was a problem writing to JL_STDOUT or JL_STDERR, so its best to try the simpelest means possible to get the error out. update doc/devdocs/stdio.rst to remove reference to: jl_write(), jl_putc(), jl_puts(). Simplify handling of STDOUT and STDERR early in initialisation: Instead of calling fwrite(), just call uv_default_loop() to get the jl_io_loop and then call jl_fs_write as usual. This means that all jl_printf()s now go through uvlib. There is a chance that this won’t work in win32, AppVeyor will tell... Remove C interface for single character writes: - jl_fs_write_byte() - jl_putc_copy() - jl_pututf8_copy() Hadnle single character writes in Julia: - write(f::File, c::UInt8) = write(f, [c]) - write(s::AsyncStream, b::UInt8) = write(s, [b]) - write(s::AsyncStream, c::Char) = write(s, string(c)) Rationale: - Writing to a libuv stream involves: "Julia does C malloc, Julia calls C, C calls libuv, libuv calls C callback, C callback calls julia hook, C callcback does c_free". Having less special cases of this makes it easier to reason about correctness. - Whatever overhead there might be in creating the temporary 1 element array (or string); it is small compared to all the machinery in libuv, and the OS that actually makes the single character write happen. - If a user is doing single character writes to a libuv stream, they either don’t care about performance, or they should be buffering in Julia before calling write(). Inline jl_write_copy() into only remaining called: jl_write(): - This is now the only place that needs to know about the trick used to store a copy of the output string after the request struct (i.e. malloc(sizeof(uv_write_t) + n)); Removed dead code: - stream.jl : make_stdout_stream() - stream.jl : jl_write_copy() Rename: - jl_write_no_copy() -> jl_uv_write() Move common code from stream.jl write() functions into uv_write macro: - ccall to jl_uv_write() (was jl_write_no_copy). - wait for current task Simplify uv_write memory management: Try to keep all the state management in one place (in macto uv_write()). Move free() of uv_write_t request object from C callback jl_uv_writecb_task() to macro uv_write(). This puts the malloc() and the free() in the same place. Move uv_req_set_data(uvw,C_NULL) from C jl_uv_write() to macro uv_write(). In _uv_hook_writecb_task() assert that uv_req_data(req) is not NULL. libuv should not call the callback from within the call to uv_write(), libuv has an internal “write_completed_queue” that is pumped by the main loop.

Make uv_write() a function (was a macro) and do c_free in "finally" clause. Explicit null-termination in jl_safe_printf() to allow for win32's non-posix vsnprintf behaviour. See also JuliaLang#9624. Use jl_safe_printf() in profile thread. Send help message to jl_printf(JL_STDOUT,) was stderr.

samoconnor · 2015-02-10T22:11:04Z

rebased again for review by @vtjnash (following jl_errorf() change by @garrison b66e5c1)

JeffBezanson · 2015-02-10T23:52:51Z

base/fs.jl

-    uv_error("write",err)
-    1
-end
+write(f::File, c::UInt8) = write(f,[c])


I would worry about the performance of allocating an array here.

Could be handled by jl_fs_write, passing &c as the data.

I would argue that this is not the right layer to deal with performance of single-byte writes.
The OS implementation of the single-byte write involves a sys-call, maybe a network stack, etc... it is expensive.
If the caller needs to efficiently write lots of single bytes, they should write to a buffer interface that wraps the raw io interface.
Is there a particular use-pattern in current Julia code where single character writes to File are performance critical?

@JeffBezanson you have to be careful there, since we could execute a task switch and invalid the stack slot before it gets written out

Hi @vtjnash,
Sorry, I'm afraid I don't quite understand.
Are you saying that the user of a buffered interface might write a character that does not get sent because it is left in the buffer? ... in that case they should call buffer.flush(). No?

that was addressed to Jeff's comment. just bad timing as we both typed replies at the same time

samoconnor · 2015-02-16T02:44:31Z

still conflict clean at this point...

JL_STD* vs ios_std* cleanup

JeffBezanson · 2015-02-16T22:35:02Z

Thanks, very nice cleanup.

ihnorton · 2015-02-16T23:03:34Z

👍

vtjnash reviewed Dec 24, 2014
View reviewed changes

vtjnash reviewed Jan 20, 2015
View reviewed changes

samoconnor force-pushed the master branch from 1906ee8 to 15c93f6 Compare February 2, 2015 23:20

samoconnor force-pushed the master branch from 15c93f6 to 97a1c15 Compare February 6, 2015 09:51

tkelman mentioned this pull request Feb 8, 2015

jl_errorf: null terminate buffer before printing #10112

Merged

samoconnor added 2 commits February 11, 2015 09:07

samoconnor force-pushed the master branch from 97a1c15 to 2af73dd Compare February 10, 2015 22:08

JeffBezanson reviewed Feb 10, 2015
View reviewed changes

tkelman mentioned this pull request Feb 12, 2015

Parse all command line options in repl.c #9482

Merged

JeffBezanson added a commit that referenced this pull request Feb 16, 2015

Merge pull request #9450 from samoconnor/master

ab5c4cf

JL_STD* vs ios_std* cleanup

JeffBezanson merged commit ab5c4cf into JuliaLang:master Feb 16, 2015

tkelman mentioned this pull request Feb 28, 2015

RFC: test 32 bit linux on travis #9153

Merged

vtjnash mentioned this pull request Apr 24, 2015

Possible Segfault With SIGINT And File IO #10990

Closed

samoconnor referenced this pull request in samoconnor/julia Jan 12, 2016

use Filesystem.File for STDIO

e4ec915

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JL_STD* vs ios_std* cleanup #9450

JL_STD* vs ios_std* cleanup #9450

samoconnor commented Dec 24, 2014

tkelman commented Dec 24, 2014

samoconnor commented Dec 24, 2014

Keno commented Dec 24, 2014

tkelman commented Dec 24, 2014

tkelman commented Dec 24, 2014

vtjnash Dec 24, 2014

samoconnor commented Dec 24, 2014

vtjnash commented Dec 24, 2014

vtjnash Dec 24, 2014

samoconnor Dec 24, 2014

vtjnash Dec 24, 2014

samoconnor commented Dec 24, 2014

vtjnash commented Dec 24, 2014

samoconnor commented Dec 25, 2014

vtjnash Jan 20, 2015

samoconnor Jan 20, 2015

samoconnor commented Feb 2, 2015

tkelman commented Feb 2, 2015

samoconnor commented Feb 2, 2015

samoconnor commented Feb 6, 2015

samoconnor commented Feb 10, 2015

JeffBezanson Feb 10, 2015

JeffBezanson Feb 10, 2015

samoconnor Feb 11, 2015

vtjnash Feb 11, 2015

samoconnor Feb 11, 2015

vtjnash Feb 11, 2015

samoconnor commented Feb 16, 2015

JeffBezanson commented Feb 16, 2015

ihnorton commented Feb 16, 2015

JL_STD* vs ios_std* cleanup #9450

JL_STD* vs ios_std* cleanup #9450

Conversation

samoconnor commented Dec 24, 2014

tkelman commented Dec 24, 2014

samoconnor commented Dec 24, 2014

Keno commented Dec 24, 2014

tkelman commented Dec 24, 2014

tkelman commented Dec 24, 2014

Choose a reason for hiding this comment

samoconnor commented Dec 24, 2014

vtjnash commented Dec 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samoconnor commented Dec 24, 2014

vtjnash commented Dec 24, 2014

samoconnor commented Dec 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samoconnor commented Feb 2, 2015

tkelman commented Feb 2, 2015

samoconnor commented Feb 2, 2015

samoconnor commented Feb 6, 2015

samoconnor commented Feb 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samoconnor commented Feb 16, 2015

JeffBezanson commented Feb 16, 2015

ihnorton commented Feb 16, 2015