Skip to content

Commit

Permalink
Refinement per @vtjnash comments.
Browse files Browse the repository at this point in the history
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 #9624.

Use jl_safe_printf() in profile thread.

Send help message to jl_printf(JL_STDOUT,)  was stderr.
  • Loading branch information
samoconnor committed Feb 10, 2015
1 parent b75971b commit 2af73dd
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 16 deletions.
24 changes: 11 additions & 13 deletions base/stream.jl
Original file line number Diff line number Diff line change
Expand Up @@ -725,26 +725,27 @@ end
# finish_read(state...)
#end

macro uv_write(stream, data, len)
esc(quote
check_open(s)
uvw = c_malloc(_sizeof_uv_write)
function uv_write(s::AsyncStream, p, n::Integer)
check_open(s)
uvw = c_malloc(_sizeof_uv_write)
try
uv_req_set_data(uvw,C_NULL)
err = ccall(:jl_uv_write,
Int32,
(Ptr{Void}, Ptr{Void}, UInt, Ptr{Void}, Ptr{Void}),
handle($(stream)), $(data), $(len), uvw,
handle(s), p, n, uvw,
uv_jl_writecb_task::Ptr{Void})
if err < 0
c_free(uvw)
uv_error("write", err)
end
ct = current_task()
uv_req_set_data(uvw,ct)
ct.state = :waiting
stream_wait(ct)
finally
c_free(uvw)
end)
end
return n
end

## low-level calls ##
Expand All @@ -754,17 +755,14 @@ write(s::AsyncStream, c::Char) = write(s, string(c))
function write{T}(s::AsyncStream, a::Array{T})
if isbits(T)
n = uint(length(a)*sizeof(T))
@uv_write s a n
return int(n)
return uv_write(s, a, n);
else
check_open(s)
invoke(write,(IO,Array),s,a)
end
end
function write(s::AsyncStream, p::Ptr, n::Integer)
@uv_write s p n
return int(n)
end

write(s::AsyncStream, p::Ptr, n::Integer) = uv_write(s, p, n)

function _uv_hook_writecb_task(s::AsyncStream,req::Ptr{Void},status::Int32)
d = uv_req_data(req)
Expand Down
2 changes: 2 additions & 0 deletions src/jl_uv.c
Original file line number Diff line number Diff line change
Expand Up @@ -605,13 +605,15 @@ int jl_printf(uv_stream_t *s, const char *format, ...)
DLLEXPORT void jl_safe_printf(const char *fmt, ...)
{
static char buf[1000];
buf[0] = '\0';

va_list args;
va_start(args, fmt);
// Not async signal safe on some platforms?
vsnprintf(buf, sizeof(buf), fmt, args);
va_end(args);

buf[999] = '\0';
write(STDERR_FILENO, buf, strlen(buf));
}

Expand Down
2 changes: 1 addition & 1 deletion src/profile.c
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ void *mach_profile_listener(void *arg)
bt_size_cur += rec_backtrace_ctx_dwarf((ptrint_t*)bt_data_prof+bt_size_cur, bt_size_max-bt_size_cur-1, &uc);
}
else if (forceDwarf == -1) {
jl_printf(JL_STDERR, "Warning: Profiler attempt to access an invalid memory location\n");
jl_safe_printf("Warning: Profiler attempt to access an invalid memory location\n");
}

forceDwarf = -2;
Expand Down
2 changes: 1 addition & 1 deletion src/task.c
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ DLLEXPORT void gdblookup(ptrint_t ip)
if (line_num == ip)
jl_safe_printf("unknown function (ip: %d)\n", line_num);
else if (line_num == -1)
jl_safe_printf("%s at %s (unknown line)\n", func_name, file_name, line_num);
jl_safe_printf("%s at %s (unknown line)\n", func_name, file_name);
else
jl_safe_printf("%s at %s:%d\n", func_name, file_name, line_num);
}
Expand Down
2 changes: 1 addition & 1 deletion ui/repl.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ void parse_opts(int *argcp, char ***argvp)
jl_compileropts.cpu_target = strdup(optarg);
break;
case 'h':
jl_printf(JL_STDERR, "%s%s", usage, opts);
jl_printf(JL_STDOUT, "%s%s", usage, opts);
jl_exit(0);
case 'O':
jl_compileropts.opt_level = 1;
Expand Down

24 comments on commit 2af73dd

@staticfloat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this commit caused a rash of issues with things like GnuTLS because it changed the return type of Base.write() from Int to UInt64. We've figured it out now however.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for the return type to be switched back to Int.

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson If that happens, could you let us know at JuliaWeb so we can synchronize our changes so as not to break things again?

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Julia-side it is standard to use Int for all lengths of arrays and indices into them and things like that, a class of values that the number of bytes written by a write request definitely falls into.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbromberger, can you write the relevant code in JuliaWeb defensively so that it will work either way? Otherwise it's hard to sync things up well (not to mention compatibility with older Julia versions).

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StefanKarpinski we just got finished tagging a new release that works around this change as follows:

const WRITE_RETURN_TYPE = VERSION >= v"0.4-" ? UInt64 : Int64

I guess we could do

const WRITE_RETURN_TYPE = typeof(Base.write("")) or something to keep it in sync?

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StefanKarpinski @JeffBezanson On second thought, I think this sort of stuff really belongs in Compat. If you change the return type of something in Base, it really should result in a corresponding Compat entry. Thoughts?

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to think too hard about how to support both return types, because it's just going to be Int.

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson ... but when? Because unless we push some defensive code out there now, that change will break GnuTLS all over again, and even after we fix it, it will be broken for anyone using 0.4 builds from 16 February through the date of the change.

I think this creates an even stronger argument for a Compat entry.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wrap the calls to write in convert(Int,write(...)) – you can even define a custom write function that does this. Then it won't matter what integer type write returns.

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in a ccall:

ccall((:gnutls_transport_set_push_function,gnutls),Void,(Ptr{Void},Ptr{Void}),s.handle,cfunction(Base.write,WRITE_RETURN_TYPE,(T,Ptr{Uint8},Csize_t)))

Perhaps a custom write function is the way to go.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be changed back in a minute. I'm fine with having a Compat entry if you want though.

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson OK, if you're pushing now, then I can make the change right away. That'll work.

@staticfloat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting case in how the dynamicism (?) of Julia makes it difficult to ensure correctness. Most of the time, we wouldn't care that the return type is wrong, but since it's in a cfunction, we do care. Personally, I think the way to go here for GnuTLS is to create a write_wrapper() that does the convert() that Stefan suggests above, and then make a cfunction of that wrapper.

If we do change function APIs like this, Compat entries are great, but I'm worried that they will be very difficult to find. Unless we cfunction everything, we don't really have a way to ensure our API contracts are correct without just causing ourselves undue pain every time we make a small change to a function that changes its type stability.

@staticfloat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we ever get annotated return types on functions this issue will change a lot I think, but until then I don't see an easy way to prevent innocent mistakes like this happening in the future.

@ivarne
Copy link
Member

@ivarne ivarne commented on 2af73dd Feb 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.4 is unstable development version, so if you don't bother writing code that works from February 16. until today, you can just check for a faulty version and tell your users to upgrade.

@sbromberger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staticfloat can you join me in JuliaWeb/GnuTLS.jl#37 for a quick discussion?

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cfunction should just try harder to ensure that return types are correct – I end up writing wrappers that have both convert calls and type assertions on the return value all the time. It seems reasonable for cfunction to just do that for you. I also think that while it's sometimes useful to get a cfunction for a particular method of an existing C function, it's much more common to define a wrapper in Julia only to get a C function for it, which is currently verbose and repetitive and could be much simpler.

@samoconnor
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the inconvenience @staticfloat, @sbromberger, that is down to poor refactoring discipline on my part. My intention was to avoid changing any user-visible behaviour.

As a Julia newby I'm a little surprised that cfunction does not guarantee that the return types is correct.
If cfunction is told to provide a function pointer to a function returning Int, it should be cfunction's responsibility to do that. If the compiler can't be sure of the return type of a particular method, then it should generate a wrapper that does the conversion.

Having written that, then reading the whole conversation above, I realise that I'm pretty much repeating what @StefanKarpinski just said.

Another thing...
Based on this: http://julia.readthedocs.org/en/latest/manual/calling-c-and-fortran-code/#type-correspondences
... it seem like the GnuTls.jl use of cfunction is NQR. Should it not be using Clonglong instead of Int64? https://github.com/JuliaWeb/GnuTLS.jl/blob/master/src/GnuTLS.jl#L314

The whole cfunction and ccall interface seems too fragile.
Maybe there is a better way using @Keno's cxx" magic. JuliaInterop/Cxx.jl#8 (comment)

What if, instead of cfunction, you just use cxx" to define an inline C function. And instead of ccall you just use icxx" to call the C function. That way, the C compiler can see the function definition of the callback function, and the call where the function pointer is passed to the C library. And, the C compiler can check the function pointer type.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, um the return type of write wasn't checked by the tests in base ?!

The not-very-satisfying answer to dynamic languages being unable to provide hard guarantees on type safety is usually "write more tests." Any time an unintended breaking change slips onto master and doesn't get found until it causes problems in packages means we have a hole in our test coverage that needs plugging.

@timholy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most tests look for equality, and we have 0x01 == 1. Of the 24000+ lines in our test suite, 171 contain calls to typeof.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps more tests should check for identity since 0x01 !== 1.

@JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented on 2af73dd Feb 19, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up for grabs issue created: #10251.

Please sign in to comment.