Null terminate strings automatically in Strs()

[splace]

Strs func claims to work with go strings,(non null-terminated) but doesn't add x00, and shadersource requires them, if length not specified.

opengl doc for ShaderSource

If length is NULL, each string is assumed to be null terminated.

[errcw]

This is a bit of a tricky case. Strs is designed as a general mechanism for interacting with OpenGL APIs, which implies support for both null-terminated strings and non-null-terminated strings. glShaderSource, for example, allows for strings that omit null terminators so correspondingly Strs does not impose the requirement that the Go string be null terminated. It might be reasonable to offer a version of this helper that attaches null terminators, say, NullTerminatedStrs.

[splace]

i think the OpenGL API's allow non-null terminated IF they get an array of lengths, makes sense, but since Strs doesn't return the lengths, (and there would be issues with two parameters anyway), how could it ever work for non-null terminated strings? like its documented to be able to.

why not just have Strs check and add Null's if not there? even just always add one, two wouldn't hurt. this was what i assumed it had to be doing, here is your example i was recommended to follow;

https://github.com/go-gl/example/blob/ee0644b7c5650555db3c0f4d04f9ef5716e6c6ac/gl41core-cube/cube.go#L178-L183

i guess the example requires pre-null terminated strings to work.

[errcw]

Yep, the example appends \x00 to the shader source, which is unfortunate.

I think you're right that it should be (relatively) straightforward to have Strs append a null terminator to the returned C strings. I'm not sure there is necessarily even a meaningful downside to unconditionally adding one extra byte.

+@shurcooL to chime in once he's back. Just want a quick sanity check from another go-gl dev before committing to this feature.

[dmitshur]

As far as I can tell, the way things work is that user passes a Go string with "\x00" suffix to gl.Strs, and then calls gl.ShaderSource with that.

As the example shows:

https://github.com/go-gl/example/blob/ee0644b7c5650555db3c0f4d04f9ef5716e6c6ac/gl41core-cube/cube.go#L254

@splace, can you clarify what the problem is? Is this a feature request? A problem in documentation? Broken functionality?

[errcw]

I believe the feature request is to unconditionally append \x00 to the C strings produced by gl.Strs such that Go developers are not required to remember to append it themselves.

[dmitshur]

Didn't we already do that via #46, go-gl/glow#71 and #49?

[errcw]

My understanding is that the request here is to revisit the decision in #46 and make Strs more friendly by adding a null terminator to the passed in Go strings.

[dmitshur]

the feature request is to unconditionally append \x00 to the C strings produced by gl.Strs

IIRC, I think it was a design decision not to modify input strings in gl.Strs because that would cause allocations, and it's less expensive for the user to pass a string with a trailing null byte.

See #44 (comment) that explains that:

I've played with a prototype version of this and we discussed it with @slimsag earlier. Back then, we considered modifying Str helper to allocate memory in C heap and return a free func. It would be a breaking API change, but we have the excuse to do that, these Cgo rules have changed and we could not have predicted them in original API design.

However, I've opted to try a different approach of creating a 2nd helper instead. My thinking was (and it's still up for discussion, this can be changed) that many existing uses of gl.Str are fine when a pointer is not taken of the resultant string, and they tend to have the "\x00" at the end. The new helper doesn't need that, so people can start opting into using it on demand (primarily as they run into panics with gl.ShaderSource).

But my memory of this is fuzzy, and it's possible I got some of the details wrong. Edit: Now that I've spent more time looking into this, most of what I said above is not accurate/relevant.

Looking at the source of Strs, it allocate a contiguous array large enough to hold all the strings' contents anyway. So it wouldn't be much more expensive for it to add nulls.

So the best reason I can think of that we didn't support this is to reduce complexity. We probably didn't see a reason why user can't pass null terminated Go string themselves; it's not hard. And it kept things more well-defined and maps to C APIs more closely.

Edit: Instead, #46 (comment) is what I was thinking of/trying to recall.

I would imagine that a version of Strs helper that accepts Go strings without null termination byte and adds them itself is best done as a separate function, and it can live outside of go-gl/gl repo in a utility package.

One of my concerns is having too many choices offered to a new user of gl. Do they use Str? Strs? NullTerminatedStrs?

Since my memory is fuzzy on the details, I'm open to corrections or hearing motivation why this would be a good change to make. But right now I don't see any motivation provided in this issue.

[dmitshur]

Sorry for trigger happy comments, I'm coming back to this after a long time. To summarize my stance on this, I'm open to discussion if additional information and motivation is provided. I want to see sample code that demonstrates how user code would benefit from the proposed change. Then we can discuss trade-offs and various options. I don't want us to make changes to gl unless there's rationale provided.

So, this issue is in "waiting for info" state for me.

[j7b]

I've bit myself a couple times in unpleasant ways with this and end up doing stuff like

if !strings.Contains(source, "\x00") {
	source += "\x00"
}

in my shader compiler cause null terminators aren't something I'm thinking about in Go, but it did occur to me things like gl.ShaderSource could check if the **uint8 is terminated if it's supposed to be and if not fix it, which wouldn't change the behavior of Strs and would eliminate the need for a Strs variant that adds a terminator.

[splace]

Just thinking....

might neaten this up by making use of the other form of the call, pointers into an array of strings, but with only the one string.

[pwaller]

Looks like this thread fizzled out. Is there anyone with a current strong opinion of what's needed?

My reading of the thread is that:

1. At the moment, users must null terminate their strings before feeding them to C, for many APIs.
   • Forgetting to do so can lead to unpleasant-to-debug crashes.
2. A couple of users requested this (I've encountered this problem myself).
3. Adding a \x00 to the end has little downside, since the string is being reallocated anyway, so "what's one extra byte between friends"?
4. There are no currently known downsides
   • Other than the work required to implement this.

If my reading is correct, are there any objections to implementing this, and is someone willing to do the work? I can't see why not, if it simplifies user code and makes things less error prone, with minimal additional runtime overhead.

If my reading is incorrect, please let me know how.

[hajimehoshi]

+1 to ensuring the string to end with \x00 on go-gl side.

[splace]

might neaten this up by making use of the other form of the call, pointers into an array of strings, but with only the one string.

[bump]

just to clarify, the idea is to have any call with a string parameter translate into a single item string ARRAY call. the API seems to generally always support both ways AFAIK.

doesn't this avoids nul termination and so is more Go'ish, not sure if it might even enable avoiding copying?

[pwaller]

It's unclear to me how an array helps, don't the individual char* within the char*[] still have to be nul terminated?

Please can you shed some light on this, perhaps with a code sample (e.g. pick a specific OpenGL call and demonstrate the two alternatives?)

[splace]

forget what i said, i seem to have been mis-reading the spec.; i guess i was thinking that (the null character is not counted as part of the string length) meant the null wasn't required if you provided the length and i assumed the API wouldn't include what is just an optimisation, (the lengths being unnecessary, only there so code could jump through strings without testing for null, but could actually just test for the nulls and ignore the lengths.) i would guess implementations might not check the null is actually there, just asking for trouble, the string might get passed on to something assuming a null.

Name

glShaderSource — Replaces the source code in a shader object
C Specification
void glShaderSource( 	GLuint shader,
  	GLsizei count,
  	const GLchar **string,
  	const GLint *length);
 
Parameters

shader

    Specifies the handle of the shader object whose source code is to be replaced.
count

    Specifies the number of elements in the string and length arrays.
string

    Specifies an array of pointers to strings containing the source code to be loaded into the shader.
length

    Specifies an array of string lengths.

Description

glShaderSource sets the source code in shader to the source code in the array of strings specified by string. Any source code previously stored in the shader object is completely replaced. The number of strings in the array is specified by count. If length is NULL, each string is assumed to be null terminated. If length is a value other than NULL, it points to an array containing a string length for each of the corresponding elements of string. Each element in the length array may contain the length of the corresponding string (the null character is not counted as part of the string length) or a value less than 0 to indicate that the string is null terminated. The source code strings are not scanned or parsed at this time; they are simply copied into the specified shader object.
Notes

OpenGL copies the shader source code strings when glShaderSource is called, so an application may free its copy of the source code strings immediately after the function returns.

[gregjohnson2017]

I had the same problem with Go strings not being null terminated. Example:

uniScrSizeID := gl.GetUniformLocation(textProgramID, &[]byte("screen_size")[0])

I spent a couple hours debugging my application as it was producing undefined behavior where my shaders would randomly not render anything. The problem is fixed by adding a \x00 to the end of the string literal for the uniform variable name.

uniScrSizeID := gl.GetUniformLocation(textProgramID, &[]byte("screen_size\x00")[0])

Most newcomers like myself to this go-gl API will probably make this mistake.

[docuys]

Can someone plse clarify the current status in a 1 or 2 liner, because as newbie I'm struggling with the 'createshader' and gl.Strs being null terminated or not issue right now. Thanks.

[dmitshur]

@docuys I suggest looking at the examples in https://github.com/go-gl/example. gl41core-cube in particular uses both gl.CreateShader and gl.Strs, see here.

[docuys]

lol! dmitshur, I actually did that and managed to get it going a couple of hours before your reply! Thank you very much for the reply in any case! (btw, I am writing in Skycoin's CX. See Skycoin/CX or Skycoin.net)

[docuys]

O, and yes, I got the shader to compile without adding any 00's (per your link that is).

[docuys]

However, notice there are 2 different functions used: gl.Str and gl.Strs. I used gl.Strs, so I will have to find out the exact differences.