Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expand global table size #811

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Conversation

ab9rf
Copy link
Member

@ab9rf ab9rf commented Nov 28, 2024

need to remember to check and update this with each release

need to remember to check and update this with each release
@myk002
Copy link
Member

myk002 commented Nov 28, 2024

is there a regression test we could add that will fail the build if it's wrong?

@ab9rf
Copy link
Member Author

ab9rf commented Nov 28, 2024

Probably could be done with a lua script at runtime: just start at global_table[0] and iterate forward until .address is null; that index is the terminator, which Toady has guaranteed us will be immediately after the last entry.

@myk002
Copy link
Member

myk002 commented Nov 28, 2024

I'll see if I can write a test like that

@myk002
Copy link
Member

myk002 commented Nov 28, 2024

testing this:

-- validates that the size in df.globals.xml is correct
function test.global_table_size()
    local elem_size = df.global_table_entry:sizeof()
    local declared_arr_size = df.global.global_table:sizeof() // elem_size
    for i=0,declared_arr_size-2 do
        expect.true_(df._displace(df.global.global_table[0], i, elem_size).address,
            'nil address found in middle of global_table')
    end
    expect.false_(df._displace(df.global.global_table[0], declared_arr_size-1, elem_size).address,
        'nil *not* found at end of declared global-table')
end

@myk002
Copy link
Member

myk002 commented Nov 28, 2024

version 2 of the test:

-- validates that the size in df.globals.xml is correct
function test.global_table_size()
    local elem_size = df.global_table_entry:sizeof()
    local declared_arr_size = df.global.global_table:sizeof() // elem_size
    for i=0,declared_arr_size-1 do
        expect.true_(df._displace(df.global.global_table[0], i, elem_size).address,
            'nil address found in middle of global_table at idx ' .. i)
    end
    if df._displace(df.global.global_table[0], declared_arr_size, elem_size).address then
        local idx_of_nil
        for i=declared_arr_size+1,declared_arr_size*2 do
            if not df._displace(df.global.global_table[0], i, elem_size).address then
                idx_of_nil = i
                break
            end
        end
        expect.fail('nil *not* found at end of declared global-table (idx ' .. declared_arr_size .. '); table size should be: ' .. idx_of_nil)
    end
end

output:

Check failed! nil *not* found at end of declared global-table (idx 366); table size should be: 379
  at ...eamapps/common/Dwarf Fortress/hack/scripts/test/core.lua:58

test failed: core:global_table_size

@myk002
Copy link
Member

myk002 commented Nov 28, 2024

need to remember to check and update this with each release

you know just how to push my buttons

@ab9rf
Copy link
Member Author

ab9rf commented Nov 28, 2024

I will point out that attempting to read past the actual last element is UB, and so it's probably safer to scan the table until you find the terminator. This will identify its actual length. Then compare with what is declared; if different, fail.

If the declared length is greater than the actual length, reading that element will read unspecified memory.

@myk002
Copy link
Member

myk002 commented Nov 28, 2024

good point. will fix

@myk002 myk002 merged commit 54c0fea into DFHack:master Nov 29, 2024
17 checks passed
@myk002
Copy link
Member

myk002 commented Nov 29, 2024

ok, the test correctly fails in https://github.com/DFHack/dfhack/actions/runs/12076775793
and passes when the df-structures ref is updated in https://github.com/DFHack/dfhack/actions/runs/12076789083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants