Byte-order handling works only on Little-Endian architectures

[acolomb]

The current code in ihex_mem_copy() assumes that bytes in a 16- or 32-bit word have to be swapped when the target is specified as big-endian. This is only true for little-endian host architectures like x86 where the library is compiled.

I propose to add compile-time checking of the host endianness to the configure script to make it work on big-endian CPUs like some ARM. However, I have yet to find a portable way of doing so.

A specifier for IHEX_ORDER_NATIVE would also be of benefit to disable byte-swapping completely, independent of the host endianness.

[martin-helmich]

Just did a quick Google search; at least GCC offers a macro __BYTE_ORDER__ that one could check against at compile time (source):

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

Not exactly portable (at least compiler-wise), though.

[acolomb]

Reading a bit about it, there seems to be no standard or portable way to do it at compile-time. Most proposed solutions work at runtime, but may be evaluated during optimization by the compiler.

It basically always involves writing something to memory (which could be a couple of bytes within the static data segment), then checking via byte-wise access. I'll try to come up with an elegant solution...
The runtime approach could still be there only as a fallback if __BYTE_ORDER__ is not defined.

[acolomb]

On Linux glibc at least, endian.h defines __BYTE_ORDER instead of GCC's __BYTE_ORDER__.

[acolomb]

Can this be reopened? The original bug still remains.

[martin-helmich]

Sure. GitHub must have closed it automatically when I merged #16.

[acolomb]

Actually reading other peoples' opinion about the whole endianness topic got me thinking why this code is really in libcintelhex.

The data parsed from Intel Hex files is essentially a big pile of bytes. Single bytes. The records can be addressed using 16-bit linear, 24-bit segment or 32-bit linear addresses. Each record's data is stored as an integral number of bytes (acknowledged by the definition of ihex_rdata_t), specified by the length field.

The API of ihex_mem_copy() mixes up the (a) copying, (b) alignment and (c) interpretation of the data.

Its destination is only a generic (void*), with a size n measured in bytes. This is all that's required for (a).

The data alignment (b) is passed in the width argument, but the bytes still end up at the (possibly non-aligned) base address specified in each record. Assuming an alignment in the Intel Hex data is wrong, each byte can be addressed individually there. So it concerns only the host alignment, but fails to actually respect it. A record specifying 4 bytes starting at address 0x02 will be copied to that destination offset even with IHEX_WIDTH_32BIT on an architecture requiring 4-byte alignment of 32-bit values.

The problem with byte-ordering is actually totally unrelated to reading the Intel Hex file. As in my application's case, libcintelhex could be used just to read binary data, possible relocate it and write it out to another file in another format. Endianness only comes into play when the binary data is (c) interpreted and used in a calculation on the host architecture. However, this is highly application-dependent.

I believe that leaving this complexity and therefore the latter two arguments of the ihex_copy_mem() function out would make the library cleaner and better abstracted. Of course, the endian-dependent interpretation would have to be moved to each existing application's logic instead, breaking compatibility.

As with network protocols for example, the endianness of each value interpreted from the binary data is only dependent on the file and its agreed content format. The host architecture and endianness don't matter when reading the data, only when trying to access it directly using pointer references to architecture-dependent types (type punning). An endian-agnostic application should always read byte-by-byte and assemble multi-byte values by arithmetic operations.

[martin-helmich]

Thank you for the elaborate comment. I'll try to wrap my head around this as soon as possible and get back to you regarding the issue.

[acolomb]

Pull Requests #17 / #19 show a possible API to avoid all byte-order problems. They are solely based on single-byte copies.

I think a separate ihex_wordXX_copy() function could cope with byte-order handling on user request (see acolomb/libcintelhex@ae38bd4). Solving #12 should be trivial then.

Note that the conversion code itself in ihex_mem_copy() is actually endian-agnostic. It will correctly assemble a uint32_t value by using host arithmetic. The o argument should just be interpreted as the byte order in the Intel Hex file.

The reason for my proposal of independent copying functions for different word-sizes is a correctness issue with the C standard. IIRC, type-punning through pointers is undefined behaviour in standard C. An array of 16-bit integers should be passed as uint16_t* in order to make assumptions about its alignment. For example: If passing a uint16_t* array through a void* and then iterating through it by sizeof(uint16_t) (or w = 2) increments is not guaranteed to work, since the 16-bit values might actually be four-byte aligned in memory! The standard-correct solution would therefore be access through a correctly-typed pointer.

[martin-helmich]

I see your point. We originally introduced this behaviour, because our target system did not support byte-wise writing and we wanted to dump the hex file into device memory in a single pass.

I think a separate ihex_wordXX_copy() function could cope with byte-order handling on user request (see acolomb/libcintelhex@ae38bd4). Solving #12 should be trivial then.

Sounds good. I posted some comments on #19, but I really like the general idea. I would actually be alright with deprecating ihex_mem_copy altoghether in favor of ihex_word{8,16,24,32,64}_copy functions.

[acolomb]

Alignment would be an issue to deal with, making sure the start offset and size are multiples of the bit width. That's a pretty simple check for the target memory range, but individual Intel Hex records actually have no alignment requirements. The "load offset address" for 8-bit, 16-bit and 32-bit can always point to single bytes (no proper alignment), and the record length is not required to be consistent with the addressing scheme (ELA / ESA).

Example: Copying a 32-bit value which was split between two ihex records would not be efficient, requiring a different approach to the copy loop.

The only case that always works is ihex_word8_copy(), although ihex_byte_copy() as an alias seems more intuitive to me since the Intel Hex format is actually defined in terms of bytes (or hex digit pairs).

I'm not sure if the 24-bit word size is actually in use on any relevant host architectures?

Maybe we can keep ihex_byte_copy() for partial copies like proposed, without any alignment requirements and add the ihex_wordXX_copy() functions with a consistent interface:

• no start offset for the target range
• target range pointer of the corresponding type uintXX_t*
• number of elements in the target range as size_t (so the target memory range is actually a uintXX_t[] array)
• specification of the ihex file's byte-order for conversion
• bail out with an error code if the ihex records don't fulfill the alignment requirements

ihex_word8_copy() could then just be a wrapper around ihex_byte_copy() with no target offset and uint8_t* instead of char*, because the latter is always a compatible pointer.

Deprecating ihex_mem_copy() in favor of the ihex_wordXX_copy() functions would also give us a chance to avoid the implicit call to ihex_mem_zero() and still be consistent and flexible.