lua5.1 compatibility

[smarek]

No description provided.

[Qix-]

For the bit functions, some verions have bit32 instead of bit.

You should have at the top something like

local bit = rawget(_G, 'bit') or rawget(_G, 'bit32') or error[[bitwise operation library not found]]

[smarek]

i intended to use BitOp, as introduced in CI tests setup

https://github.com/smarek/ci_targets/blob/master/prepare-lua#L22
https://bitop.luajit.org/

Is that snippet still correct?

[ildar]

Looks like this whole PR can be simplified with using https://github.com/keplerproject/lua-compat-5.3/

[Qix-]

A number of points:

• Not everyone has luarocks available, nor is luarocks compatible with all cases of Lua. Please don't make this assumption. If it must be used in CI, this is code smell for something not being available in the repository that users would otherwise need, and thus creates the dependency on Luarocks by end users. Testing (test libs, runtimes, etc.) is the only exception to this rule.
• Lua 5.1 and above all have bitwise operations and/or the bit[32] library. This should be sufficient for the use-cases here. Please don't introduce a dependency for it.
• Please don't use compat libs. They usually overwrite globals (though I know Keplar does not) and are NOT compatible with all environments where Lua is used. Further, I don't think 5.1 is missing anything that the Kaitai runtime needs aside from string.unpack, which is already ponyfilled (rightfully so).

As someone who is currently blocked (using kaitai) by lack of 5.1 support in this particular case (and have to resort to using hand-rolled parsers still), any of the three above things would still prevent me from using Kaitai even after this PR lands.

[smarek]

@Qix- points taken

• i do not expect luarocks to be available, only use them in testing (ci_targets) but since the luautf8 and luabitop rocks dependencies, i wonder if we can include them in runtime directly
• bitwise operations
  • from docs http://lua-users.org/wiki/BitwiseOperators
    • 5.0 => bitlib
    • 5.1 => BitOp (library)
    • 5.2 => bit32 (native)
    • 5.3 => bitwise operators (& | >> << etc.)
  • i suppose supporting Lua 5.0 does not make sense anymore, but 5.1+ does, so either it's bit (library) or bit32 (native) interface we want to use
  • for 5.3 we still have to use bit (library) because we cannot support the native bitwise operators in older versions

Is the bitwise support matrix right, or did i miss anything? Because you say Lua 5.1 has built-in bitwise operations, but I don't think it's true. Also I'm not sure about support in LuaJIT and whether we wanna support that too

[ildar]

Let me add my 10 cents 1. I referenced LuaRocks only as a big catalogue of Lua modules. A developer doesn't have to install or use it. The proposed code is starting to use external libraries. So a developer has to install them manually or with any tool he likes. 2. On bitwise operations: compat-5.3 library works on Lua 5.3 (haha!) so it may allow one code for all versions. 3. @qix : general assumptions of "badness" of compat libraries should better be confirmed by issue ## or discussion references. Ideally --- by practice.

[smarek]

In my opinion, if anything, I'd like to replace all compiled dependencies (such as compat or lua-utf8) with pure-lua dependencies. Those can be easily included in runtime directly, and will allow for better cross-compatibility without need to install external libraries in platform-specific way (be it luarocks or apt)

I initially made a mistake presuming luarocks is standard way of dependencies management, included all on all kinds of platforms with lua support, but now I think we should use luarocks to install only test dependencies (luaunit and luafilesystem) and all the others have included in runtime directly.

So, given this, I'm actually against using compat53 library, as it needs to get compiled for target platform, and the same can be achieved by pure-lua probably.

[ildar]

I like your point of view. But I doubt about implementing (missing) bitwise operations in pure Lua. Do you have an example or idea?

[smarek]

@ildar well from wiki http://lua-users.org/wiki/BitwiseOperators
it seems there are several pure-lua implementations, and since we will always use the bit or bit32 interface, it should be doable
Only thing I've not yet done, is testing whether the interface methods are same or have differences (number and order of arguments, function names)
Most promising is probably this one? https://github.com/davidm/lua-bit-numberlua/

[ildar]

Indeed.

https://github.com/davidm/lua-bit-numberlua/

Looks good.

[Qix-]

i suppose supporting Lua 5.0 does not make sense anymore, but 5.1+ does

Correct. 5.1+ is pretty typical due to LuaJIT's freeze at 5.1.

o either it's bit (library) or bit32 (native) interface we want to use

Please, no libraries.

local function try_require(name)
    local success, mod = pcall(require, name)
    if success then return mod else return nil end
end

local bit = rawget(_G, 'bit32') or rawget(_G, 'bit') or try_require('bit') or try_require('bit32') or error[[no bitwise library found]]

print[[bit library found]]

$ lua5.1 bit-test.lua
bit library found

$ lua5.2 bit-test.lua
bit library found

$ lua5.3 bit-test.lua
bit library found

for 5.3 we still have to use bit (library) because we cannot support the native bitwise operators in older versions

Correct.

Is the bitwise support matrix right

No. As I mentioned in my previous comments, Lua 5.1, 5.2, and 5.3 all have everything Kaitai would need aside from maybe utf-8 support and string.unpack, depending on what it needs there.

[smarek]

@Qix- i'm still not sure, whether clean Lua 5.1 has bit/bit32 interface? Why else would BitOp exist, if said versions contained built-in bitwise operations?

[Qix-]

Strange. I downloaded the source from lua/lua and compiled myself. It appears the Ubuntu distribution comes with bit installed but the PUC Rio (mainline) sources do not. Sounds like some Debian package manager needs to be yelled at, but that's a different issue.

I think most people working with 5.1 are using LuaJIT or are in an environment where bit exists.

I think the best solution here, personally, is to indicate that for 5.1 support, you need to provide bit or bit32 by way of require() yourself, whether that be provided by the implementation of Lua you're using or by importing some library (e.g. BitOp). We shouldn't provide it here as I agree that setting up compilation is going to be a worse idea.

Just a line in the documentation, probably bolded. Let's assume that bit or bit32 are available either as globals or as require()-able modules.

---

If we want to be really respectful of platforms/environments/etc, don't even check global - go straight to the require. That creates a very solid contract.

local function try_require(name)
    local success, mod = pcall(require, name)
    if success then return mod else return nil end
end

local bit = try_require('bit') or try_require('bit32') or error[[no bitwise library found]]

print[[bit library found]]

[smarek]

@Qix- yes, I've already found mentions of Debian lua maintainers adding the bit extension to versions where it isn't native, so I'm less surprised than you probably.
Even tho, it might not be Debian maintainer fault, as lua-users BitwiseOperators mention backporting as the reason.

As of version 5.2, Lua ships with the library [bit32] that adds support for bitwise operations. Previous versions of Lua did not include bitwise operators, but bit32 has been backported to version 5.1. Alternatively, there are Lua libraries for this as well as some patched versions of Lua.*

Anyway, there is one more issue with those, and that is potential interface inconsistency. Is bit interface consistent with bit32? Is bit32 interface consistent with BitOp?
And are those two consistent with https://github.com/davidm/lua-bit-numberlua/ ?

I'm thinking about writing some kind of sanity tests that would run before any of these methods are called, and would fail out hard, if the bit/bit32 interface found locally is not producing expected results, or the calls to the interface methods fail, because number/order of arguments differ from our expectations.

And that whole possible mess, i've just described, motivates me to just use some library, such as mentioned lua-bit-numberlua, pack that along in the runtime, and not worry about any platform-specific modifications/issues.

@Qix- do you think i worry too much, or how would you implement the precautions if you kinda agree with me?

[smarek]

Elaborating a bit more, in language where require('bit') might require 30 different file locations, be it user test script with the same name, to part of library, that is not at all providing bitwise operations method, i'm quite worried

Maybe, this kind of explicit require/mention of individual interface methods would guarantee that at least the correct methods exist in the imported interface/module?

local bnot = bit.bnot
local band, bor, bxor = bit.band, bit.bor, bit.bxor
local lshift, rshift, rol = bit.lshift, bit.rshift, bit.rol
-- etc...

source: https://bitop.luajit.org/api.html#shortcuts

[smarek]

I've updated PR with code for requiring utf8 and bit/bit32 modules, please let me know what you think

[smarek]

@Qix- I've tried to fix the whole UNICODE/UTF/CP437/SJIS/... mess, would you be please so kind, and looked at the changes in be0ed12 ?
The Unicode module is a bit modified version from NMAP
And I think it should be relatively simple to extend it for SJIS (CP932) code-table, ie. by using this https://uic.win/en/charset/show/cp932/

[Qix-]

Just a few suggestions.

Along with the above comment, I think you'll get a little bit of a performance increase (in non-LuaJIT environments) by caching bor, rshift, etc. since Lua does not optimize ASTs.

[smarek]

Just a few suggestions.

Along with the above comment, I think you'll get a little bit of a performance increase (in non-LuaJIT environments) by caching bor, rshift, etc. since Lua does not optimize ASTs.

I'll probably do that as well, maybe not in current release, i'm actually thinking of creating separate todolist, that we can both work on independently, once this large changes are merged

---

One issue, that might be interesting to you @Qix- is the issue of de-coding 8-byte signed numbers so it works on all of the platforms we want the runtime to work on
iryont/lua-struct#3
struct.lua is taken directly from this repository, and even tho I tried to solve the issue, it's obviously far beyond my knowledge

It currently fails some of the tests as well, but it's easier to write some short test, than to run the whole kaitai test suite, but I'll leave that up to you, if you'll want to participate on this

[Qix-]

Losslessly storing 64-bit numbers in pure Lua is impossible without a bignum type, and even then I'd wager it's more work than it's worth (see my comment on your linked issue).

[smarek]

Ok, so we're left with two options, fail hard for reading 64bit numbers (s8be, s8le, u8be, u8le) or fail if the read value is bigger than what platform can handle (eg. reading 53/54 bits will yield correct number, reading more will fail hard)?
Or is there better solution?

[generalmimon]

Losslessly storing 64-bit numbers in pure Lua is impossible without a bignum type

Ok, so we're left with two options, fail hard for reading 64bit numbers (s8be, s8le, u8be, u8le) or fail if the read value is bigger than what platform can handle (eg. reading 53/54 bits will yield correct number, reading more will fail hard)?

Note that this is not a new topic, the same problem has been also encountered by JavaScript, see kaitai-io/kaitai_struct#183.

[smarek]

@generalmimon so it's about decision affecting whole kaitai environment? I still think, it should be declared, whether we

a. expect the target runtime to fail hard if the requested data type cannot be represented/worked with correctly (s8le/be, u8le/be methods will fail, regardless of the read data size)
b. expect the target runtime to fail hard, only if the read value cannot be represented correctly (ie. returning uint32/int32 if the read value fits, and failing otherwise)
c. expect the runtime to provide, if necessary, additional data type to handle such data (java BigInteger, lua BigNum, etc.), even if that means introducing new runtime dependency or target OS/platform constraints

[ildar]

A minor note: The top-level ‘unpack‘ function in 5.1 standard is usually associated with _G.unpack ( which is now table.unpack). So i suggest using ‘local sunpack‘ instead to avoid confusion.

[smarek]

Gentlemen, seems i've done everything i could, so it's now ready to merge

This PR is tightly related to https://github.com/kaitai-io/kaitai_struct_compiler/pull/206/files
which is required for the 5.1/5.2 targets to work (bit manipulation, encoding unicode literals)

I'd like to solve all the mentioned issues in separate tickets/developments (bignumber, wider unicode/utf8 support, performance optimizations, lua-bit-numberlua instead of native/BitOp ), if we may

Also current statistics about tests, ran by different Lua versions with runtime in this PR

> lua -v
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
# Ran 210 tests in 0.031 seconds, 194 successes, 10 failures, 6 errors

> lua -v
Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
# Ran 210 tests in 0.039 seconds, 194 successes, 10 failures, 6 errors

> lua -v
Lua 5.3.5  Copyright (C) 1994-2018 Lua.org, PUC-Rio
# Ran 210 tests in 0.049 seconds, 195 successes, 9 failures, 6 errors

Both Lua and modules (luaunit, lfs and bitop) installed through hererocks and subsequently luarocks

[ildar]

Congratulations!
Yet the dot here must be a typo?

[smarek]

Similar uses utf8.lua for relative require, and the unicode module is relative to string_decode.lua in the same directory currently, so it seemed appropriate
https://github.com/Stepets/utf8.lua/blame/master/README.md#L54

currently works even with tests, so I'm not sure it's wrong, however that might change with implementing #11

[ildar]

I see. The dot isn't for relative something, it's for overriding the built-in module.

[smarek]

@ildar I thought that was for import path/precendence, we certainly do not want to import different unicode module, than our own. Still does not do any harm, so I'll probably keep it now.

[generalmimon]

I'm not sure how this mess happened (you probably took the read_bits_int definition from the Johnnynator/kaitai_struct_luajit_runtime repo), but the result is wrong. You completely dropped the read_bits_int_le method (introduced in fc9556d from Apr 28, 2020 0:27), and this new read_bits_int method is only a duplicate of the existing read_bits_int_be method when it didn't have changes from cb6138b commited on Apr 28, 2020 0:13 yet.

Context: in 0.8 and older versions of KS, there had been only the big-endian version of bit-sized integers, so there had been only one method – read_bits_int. However, in 0.9 along with the advent of little-endian bit integers (kaitai-io/kaitai_struct#155), it was necessary to add another function read_bits_int_le for reading bit integers in LE manner. To maintain naming consistency, the old read_bits_int for reading BE bit-integers was renamed to read_bits_int_be, and in order not to break backward compatibility (to make parsing codes generated by KSC ≤0.8 work with 0.9 runtime library), the outdated method read_bits_int has been kept as a deprecated alias of read_bits_int_be. See kaitai-io/kaitai_struct#155 (comment) for more info.

In short, the canonical names are read_bits_int_be and read_bits_int_le. The name read_bits_int became deprecated (it's ambiguous now), and it'll be removed in a future release.

Anyway, this is a side-by-side comparison of your read_bits_int_be and read_bits_int methods (these are the same changes as introduced in cb6138b):

Please see how read_bits_int and read_bits_int_le methods are currently implemented on master branch and rewrite them using the bit module from scratch.

That reminds me that I have to run tests locally before I can mark this PR as OK.

[smarek]

Yes, I took a lot of code over from luajit_runtime repo, but there must have been reason for me to do it this way, it's hard to remember after few months since i've done it.
Strangely, this did not mess up any tests in place

Also no, i'm not going to rewrite the functions using bit module from scratch now, but we can surely add that task to backlog of other issues, this runtime has.

[generalmimon]

there must have been reason for me to do it this way

I really doubt that. It's obviously wrong, please read my above comment.

Also no, i'm not going to rewrite the functions using bit module from scratch now, but we can surely add that task to backlog of other issues, this runtime has.

Sorry, maybe I wrote it ambiguously, I meant copying the read_bits_int and read_bits_int_le method implementations and only replacing the ops & and >> with the bitmodule as you've done. Rewriting anything from scratch is undesirable, sorry for this wrong phrase.

[smarek]

Yeah, ok, that's something i can do for sure, no hard feelings 👍

[generalmimon]

Strangely, this did not mess up any tests in place

It certainly would now. How would possibly pass the test bits_simple_le when you completely removed the read_bit_ints_le method that it depends on?

[smarek]

Yeah, that's what happens, if my contribution from 13th is compared with updated tests/expectations from 23rd August
Merge this PR and the connected ones (specifically kaitai-io/ci_targets#7 and kaitai-io/kaitai_ci_ui#8 ), then we can work on fixing remaining issues and releasing a new version

[generalmimon]

Yeah, that's what happens, if my contribution from 13th is compared with updated tests/expectations from 23rd August

That's not true. Expectations on this matter have been last updated with these commits:

• kaitai-io/kaitai_struct_lua_runtime@fc9556d implementing the method read_bits_int_le (from April 28, 2020 0:27)
• kaitai-io/kaitai_struct_compiler@746e34e making the compiler output read_bits_int_{be,le}() calls in the generated Lua parsers (from April 29, 2020 21:17)
• kaitai-io/ci_targets@27d5f3b with the CI rebuild of bits_simple_le.lua parser code based on test KSY spec bits_simple_le.ksy using KSC having the change above - it shows that read_bits_int_le method is already in use (commit from April 29, 2020 21:40)

If this PR would be based on a earlier commit than fc9556d (imaginary situation), Git would not allow to merge this PR automatically, and merge conflicts would occur instead.

[smarek]

Sure, you're right, hopefully you feel satisfied

[smarek]

Fixed with 6f1e309