Skip to content

Commit

Permalink
Cstring: when complaing about string containing NUL, show string.
Browse files Browse the repository at this point in the history
  • Loading branch information
@StefanKarpinski
StefanKarpinski committed Jun 1, 2015
1 parent 8ef420c commit 4af6b7e
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion base/c.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ containsnul(p::Ptr, len) = C_NULL != ccall(:memchr, Ptr{Cchar}, (Ptr{Cchar}, Cin
function unsafe_convert(::Type{Cstring}, s::ByteString)
p = unsafe_convert(Ptr{Cchar}, s)
if containsnul(p, sizeof(s))
throw(ArgumentError("embedded NUL chars are not allowed in C strings"))
throw(ArgumentError("embedded NUL chars are not allowed in C strings: $(repr(s))"))
end
return Cstring(p)
end
Expand Down
2 changes: 1 addition & 1 deletion base/string.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1722,7 +1722,7 @@ wstring(s::Cwstring) = wstring(box(Ptr{Cwchar_t}, unbox(Cwstring,s)))
# to have WString
function unsafe_convert(::Type{Cwstring}, s::WString)
if containsnul(s)
throw(ArgumentError("embedded NUL chars are not allowed in C strings"))
throw(ArgumentError("embedded NUL chars are not allowed in C strings: $(repr(s))"))
end
return Cwstring(unsafe_convert(Ptr{Cwchar_t}, s))
end
Expand Down

16 comments on commit 4af6b7e

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 4af6b7e Jun 2, 2015

Choose a reason for hiding this comment

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

This seems like it'll make the call to unsafe_convert more expensive due to the creation of a GC frame? Maybe that doesn't matter give the cost of the containsnul test?

@yuyichao
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't creating an ArgumentError with string as argument already create a GC frame?

Also #11508 should reduce the cost....

Edit: I guess it's a constant string so it didn't create a GC frame....

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where it might be better as:

    checkfornul(p, sizeof(s), s)
    return Cstring(p)
end

checkfornul(p, len, s) = throw(ArgumentError("embedded NUL chars are not allowed in C strings: $(repr(s))"))

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to eliminate the GC frame. My impression was that there was work in progress to more generally solve this problem.

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

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

However, my approach works, and is rather easy to understand, isn't it?

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably irrelevant but you appear to have deleted the actual NUL check.

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops!!! Yes, that was meant to have been something more like:

checkfornul(s, len) = containsnul(s, len) && throw(ArgumentError("embedded NUL chars are not allowed in C strings: $(repr(s))"))

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that helps since the check function is called unconditionally and creates a GC frame.

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't have thought that just calling a function would create the GC frame... I thought that it required allocating something, like creating the ArgumentError... I'll have to go back and look at the @code_native output...

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

This function has the exact same properties as the function it's replacing – if unsafe_convert has a GC frame in the current version, why would your checkfornul function not have a GC frame?

@yuyichao
Copy link
Contributor

Choose a reason for hiding this comment

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

You need noinline.

This is also the transformation I'm trying to di in my other PR...... (which sadly does not cover this case though)

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't you need @noinline and to have the function only be called if the NUL is found?

@yuyichao
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes. I didn't noticed that the check function is called unconditionally and the logic is in the check function.

Yes you need a error throwing function that is specialized, not inlined and only call3d on the error path

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so the test should stay in-line (better for performance), but the function that does the throw is marked as @noinline. I'll need to add @noinline to my error function (but I didn't see it being in-lined anyway looking at the code).

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 4af6b7e Jun 2, 2015

Choose a reason for hiding this comment

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

it's actually the string("", repr(s)) calls that had me concerned that this would add a GC frame. throw itself does not allocate a gc-frame, but the result of repr(s) needs to be rooted in the gc-stack before the call to string can happen.

@yuyichao
Copy link
Contributor

Choose a reason for hiding this comment

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

@vtjnash The throw local gc frame PR I had create a local gc frame for evaluating the argument of throw so it includes evaluation of string, repr etc. It should also be fine if the error path is isolated in a @noinline function that does all the allocation. And yes, throw (is special cased) to not need it's only argument rooted at all.

Please sign in to comment.