Add more Writer functionality

[jchv]

• Adds SeekableBuffer. This is because io.Buffer does not implement io.Seeker unfortunately, but it will be useful in some circumstances to be able to buffer serialization inside of generated code.
• Adds ProcessZlibCompress, just the write side of ProcessZlib.
• Adds helpers for serialization code that will need seeking. Not all structs will require seeking, although we could opt to just force the user to always use an io.ReadWriteSeeker as an alternative, in which case we definitely want to have the SeekableBuffer.
• Adds write-side functionality for bits and terminated/padded byte strings.

Since Kaitai Struct doesn't yet emit code that uses the Writer interface, I figured it would probably be a good time to start shaking things out. I used my own experimental implementation of Kaitai Struct to try to figure out what functionality would be needed to implement all of serialization, and I think this is about it.

I've chosen to try to maintain symmetry with the reader side, which means there's some less than idiomatic names here.

[generalmimon]

uint is 32 bits wide on 32-bit platforms, see https://go.dev/tour/basics/11.

Suggested change

	val &= (1 << uint(n)) - 1
	val &= (1 << uint64(n)) - 1

[jchv]

The important thing is that it needs to be unsigned, since I doubt we will need to worry about bit-sized types with more than 2^32-1 bits. (Though if we want to support more than 64 bits, it will be a lot more complex. But Java already assumes the number is <= 64). This doesn't impact the type inference: it will still get uint64 because of the left side of &=.

[generalmimon]

This doesn't impact the type inference: it will still get uint64 because of the left side of &=.

Oh right, I didn't realize that this was the shift distance and not a regular operand.

The important thing is that it needs to be unsigned,

I actually disagree with this part - there is no reason it needs to be unsigned. Go happily accepts int, as demonstrated here:

kaitai_struct_go_runtime/kaitai/stream.go

Line 462 in 5fde04b

var mask uint64 = (1 << n) - 1 // unlike some other languages, no problem with this in Go

So I think that uint(...) everywhere is completely unnecessary - it only clutters the code. On top of that, if what you're casting to uint happens to be negative (in case there was some bug), there's a risk of an integer overflow, as the gosec linter points out in CI:

  Error: kaitai/writer.go:284:28: G115: integer overflow conversion int -> uint (gosec)
  		mask := uint64((1 << uint(k.bitsLeft)) - 1) // `bitsLeft` is in range 0..7
  		                         ^

This could mask certain bugs. In comparison, if we just skip the uint(...) casts and perform 1 << -2, we get a panic: https://go.dev/play/p/SwyyQlzJdmY

panic: runtime error: negative shift amount

goroutine 1 [running]:
main.main()
	/tmp/sandbox2225754398/prog.go:7 +0x9

Program exited.

Which I'd argue is completely fine (because this is clearly a bug, so at least let it fail loudly and clearly), and it's better than silently getting 1 << 18446744073709551614.

[jchv]

Sure, it ultimately comes down to whether you want broken things to crash loudly or silently give bad output. I do generally agree with "crash loudly", but in this case I opted for "silently give bad output" because that is consistent; it will silently give bad output with values above 64 as well. Maybe it would be better, in fact, to explicitly check the input domain and output a specific error rather than panic. But unless I am just missing nuance I didn't see that in the Java version either, which I think also assumes that the bits are <= 64. Either way I'm none too picky about it. Whatever we want to do here is no big deal to me. I've removed the casts and have everything still working.

[generalmimon]

@jchv:

I've removed the casts and have everything still working.

Thanks! I also don't really have strong opinions on this. It's just that I don't think the uint() casts would improve the situation; if anything, I feel like they make it slightly worse (at the very least, they confuse readers of the code like me).

Actually, the original type of the n parameter was uint8. This was sufficient, because the valid range is indeed n >= 0 && n <= 64 and anything outside this range is invalid (although we don't currently enforce this). Values n > 64 don't make sense because the result is returned as uint64, so it's impossible to support reading/writing integers wider than 64 bits.

Nevertheless, I changed the type of n from uint8 to int in a5c5c1e. To be honest, I don't remember exactly why I made that decision - perhaps to align it with runtime libraries in other languages like C++: kaitaistream.cpp:409 Maybe changing it back to uint8 would make sense, which could potentially be done in other languages as well... 🤔 (however, this is a separate issue, so if you decide to tackle it, please don't do so this PR)

[generalmimon]

Stopped reading at that - I don't see any reason not to port the existing Java runtime's writeBitsIntBe or Python runtime's write_bits_int_be implementations line by line.

I'm deeply convinced that it's a big mistake to have different implementations for the same task across runtime libraries. Unique implementations only lead to unique bugs, as we've already seen in the past: #22. They have literally no benefit: maintainability is much worse.

Ideally, the implementations should be identical line by line when you compare any pair of runtime libraries: for example, you can put Java runtime's writeBitsIntBe and Python runtime's write_bits_int_be side-by-side and compare them (note that even the variable names and blank lines match - this is intentional). I want to be able to do the same with Go vs. Java or Go vs. Python.

[jchv]

I rewrote it to follow the Python implementation as closely as possible. The only major difference is I don't have the ensureBytesLeftToWrite parts implemented, because I don't think this maps to how Go abstracts I/O.

[generalmimon]

No, this is not the way - there has to be only one writeAlignToByte() (Java) / write_align_to_byte() (Python) method that can handle both big-endian and little-endian order. The explanation is in kaitai-io/kaitai_struct#1070.

This is another instance of the "big mistake" of unique implementations I mentioned earlier - please follow the Java or Python runtime library exactly.

Only in exceptional cases, if you believe a very good reason to do something differently in Go, you should leave a comment explaining you are doing so.

[jchv]

Sure, that's fine. I removed AlignToByteLe and track the last bit endianness like Python and Java do. Also, I have made it so all of the write functions and the seek function call AlignToByte.

[jchv]

It has occurred to me that Close() probably needs to flush the bits, if the intention is that the codegen will generally not directly AlignToByte. (I'll try to fix this but I'm having some issues with GitHub atm.)

Nevermind: since it does not take a Closer, it can't. Making it take a WriteCloser would work, but would be awkward, as it is sensible to have things (e.g. in-memory buffers) that you serialize structs to that are not a Closer. So... The generated code will still need to AlignToByte at the end of a struct if it ends on a bit sequence.