`std::math::accumulate` bug with size>1

[galkinvv]

~Using imhex version is 1.33.2 I tried to use std::math::accumulate to verify the entire file u8 sum equals the value required b a format I'm investigating. However I found a bug that std::math::accumulate returns zero if the end points "just after the file end". I can sum "all file bytes except last", but can't sum "all file bytes".~ seems already fixed in nightly

I looked at source code near https://github.com/WerWolv/PatternLanguage/blob/ImHex-v1.33.2/lib/source/pl/lib/std/math.cpp#L178 but failed to find the reason.

However, looking at source code I find that it has another bug: the end adress itself is divided by size, instead of dividing the adress difference. So summing from non-zero adress of non-single-byte elements fails.

Here are sample input and output as text on a simple file consisting of 16 identical bytes 0x01

#include <std/math.pat>
#include <std/io.pat>

std::print("0x{:X}", std::math::accumulate(0, 0x1, 1)); //sum single byte
std::print("0x{:X}", std::math::accumulate(0, 0x2, 1)); //sum a pair of bytes
std::print("0x{:X}", std::math::accumulate(0, 0xE, 1)); //sum all bytes except last two
std::print("0x{:X}", std::math::accumulate(0, 0xF, 1)); //sum all bytes except last
std::print("0x{:X} WRONG", std::math::accumulate(0, 0x10, 1)); //sum all bytes - FAILS
std::print("0x{:X}", std::math::accumulate(0, 0x11, 1)); //out-of-file acess, should fail

std::print("0x{:X}", std::math::accumulate(0, 0xE, 2)); //sum 7 u16s from start
std::print("0x{:X} WRONG", std::math::accumulate(2, 0xE, 2)); //sum 6 u16s, skipping one from start - WRONG result

I: 0x1
I: 0x2
I: 0xE
I: 0xF
I: 0x0 WRONG
I: 0x0
I: 0x707
I: 0x505 WRONG
I: Pattern exited with code: 0
I: Evaluation took 0.0106068s

I dont know what causes the bug with handling the last file byte, but the "non-zero start and >1 size" seems to be fixable by something like replacing

u128 endAddr = end / size; by u128 endAddr = start + ((end - start) / size);

[paxcut]

I just tested the pattern provided on an input of 16 0x01's on the master branch (1.33.0) and got these results:

I: 0x1
I: 0x2
I: 0xE
I: 0xF
I: 0x10 WRONG
I: 0x10
I: 0x707
I: 0x505 WRONG
I: Pattern exited with code: 0
I: Evaluation took 0.050773s

so the bug that made output 5 and 6 zero seems to be fixed already. The proposed fix is not correct and there seems to be at least two problems in the current code. The variable called endAddr is really the number of times the loop needs to be executed. start and end are absolute addresses in bytes but if size is not one, we want to read values that are not bytes starting from address start up to, but not including address end. The number of (potentially-non-byte) values to read is just (end-start)/size with no start + added to it (for example, reading one 2-byte number from address 2 to address 4 would become (4-2)/2=1. If we add start then we would read 3 values instead as 2+(4-2)/2=3).

The other problem is that the address needs to be incremented by size as well at every step of the loop. I ran tests where the input file was 0x1,0x2,0x3,0x4,... and using size 2 got the sequence 0x201,0x503,0x906,0xE0A,.... The second number is 0x201 + 0x302, but what we want (this is just my assumption as I don't really know what the function is supposed to compute) is 0x201 + 0x403 or 0x604.

Based on that evidence I believe that the corrections should be: u128 endAddr = end / size; by u128 endAddr = (end - start) / size; for (u128 addr = start; addr < endAddr; ++addr) { by for (u128 addr = start; addr < endAddr; addr += size) {

[galkinvv]

@paxcut Great thanks! I didn't tried master since it had pattern editor GUI bugged for me

Renamed this bug to only track "size > 1" case

However regarding the theoretical fix - looking with a fresh eyes - it seems to be another way: endAddr is just not needed at all: iterating from start to end would be fine! for (u128 addr = start; addr < end; addr += size)

[paxcut]

Like I mentioned above, I am not sure which of two cases this function is supposed to return so I got myself confused and mixed both cases together. One case is when the loop overlaps numbers in which case a loop step of one is needed, but that seems to me to be of little use so I'm guessing that the loop step of size is the correct case. If you use for (u128 addr = start; addr < end; addr += size), then the loop is executed (end-start)/size times, exactly as it should. Nice catch! So there are two bugs currently, one with using the wrong end and the other is using the wrong loop step and both become apparent only when size >1.