sierrafoxtrot/srecord

Wrong srec_info output when using the last 32-bit address

Closed this issue · 4 comments

srec_info prints the last 32-bit address wrongly in address range end.
This is a regression introduced between version 1.64 and 1.65.0.
Note: srec_info can generate data itself, but that would only work for this first case, it wouldn't show the misbehavior of the second case below.

srec_cat -generate 0xFFFFFFFF 0x100000000 -constant 0 -header "" -start-addr 0 | srec_info

Output is:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   FFFFFFFF - FFFFFFFFFFFFFFFF

Output should be:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   FFFFFFFF - FFFFFFFF

Additionally srec_info doesn't fill other data ranges up to 8 nibbles if the last 32-bit address is used.
This is not a regression, it was already the case with version 1.64.

srec_cat -generate 0x0 0x1 -constant 0 -generate 0xFFFFFFFF 0x100000000 -constant 0 -header "" -start-addr 0 | srec_info

Output is:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   0000 - 0000
        FFFFFFFF - FFFFFFFFFFFFFFFF

Output should be:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   00000000 - 00000000
        FFFFFFFF - FFFFFFFF

In the newly introduced verbose mode, the additional fields also can't cope with the last address.

srec_cat -generate 0x0 0x1 -constant 0 -generate 0xFFFFFFFF 0x100000000 -constant 0 -header "" -start-addr 0 | srec_info -verbose

Output is:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   0000 - 0000 (0001)
        FFFFFFFF - FFFFFFFFFFFFFFFF (FFFFFFFF00000001)
Filled: FFFFFFFF00000002
Allocated:   inf%   Holes:  -inf%

Output should be:

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   00000000 - 00000000 (00000001)
        FFFFFFFF - FFFFFFFF (00000001)
Filled: 00000002
Allocated:  0.00%   Holes: 100.00%

Compare with the following command using the second to last address with the correct output already.

srec_cat -generate 0x0 0x1 -constant 0 -generate 0xFFFFFFFE 0xFFFFFFFF -constant 0 -header "" -start-addr 0 | srec_info -verbose

Format: Motorola S-Record
Execution Start Address: 00000000
Data:   00000000 - 00000000 (00000001)
        FFFFFFFE - FFFFFFFE (00000001)
Filled: 00000002
Allocated:  0.00%   Holes: 100.00%

[EDIT - I wrote this a bit too quickly, I thought I saw an implicit cast to a signed type in there, but probably wrong.]

main.cc:154 may be problematic:

            unsigned long lo = tmp.get_lowest();
            snprintf(buf, sizeof(buf), "%0*lX", prec, lo);
            std::cout << buf << " - ";
            unsigned long hi = tmp.get_highest();
            number_bytes += hi - lo;
            snprintf(buf, sizeof(buf), "%0*lX", prec, hi - 1);

get_lowest() returns a "data_t" value, typedef'd as a uint32. Cool. But that snprintf uses an unsigned long which can happen to be a uint64.
It's hard to use generic formatters like %lX when using typdef'ed datatypes... The anal way would be to have explicit uint32 / uint64 everywhere, and use the awful "%PRIU32" formatters (those are C99 <inttypes.h> , I hope modern c++ has the equivalent ?)

jtxa commented

It took me a while to understand the problem, because neither of the examples worked with the old srec_cat versions. So this is what the endless loop looked like, before being fixed in #6.
So the essence for me is: That patch was initially developed on 1.64 where it worked correctly and now after rebasing to 1.65 the output is wrong.

I did backport the patch and tried out the first example command line.
The first commit it fails is 16cf9cd (Fix error length represenation in verbose srec_info output. Dropped range as it added no value and was confusing., 2022-10-13).

The code snippet in @fenugrec's post is the right one. The hi address gets 0.
And the unsigned long on 64-bit Linux (not on 32-bit Linux and not on Win32/64) is a 64-bit data type. so the result of the substraction is an integer underflow which result to FFFFFFFFFFFFFFFF instead of FFFFFFFF.

Replacing all unsigned long by uint32_t solves the problem for me. But I would also cast any arithmetic operation (like such substractions) to avoid any kind of integer promotion.

jtxa commented

Please have a look if PR #14 fixes all problems.

It took me a while to understand the problem, because neither of the examples worked with the old srec_cat versions.

I used srec_cat to generate the input data because of these reasons:

  • more readable than some input file format
  • easier handling without having to create an input file

For testing I simply used srec_cat from current master (bugs fixed) and only used srec_info from the commit to be tested.