binutils: Use loops in `byte_get_*_endian` functions instead of switch cases.
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
Commit Message
Switch cases are used to read a value with a given size, but
these functions could be rewritten using a small loop instead,
avoiding code repetition.
This is already done in `byte_put_*_endian` functions.
binutils:
* elfcomm.c: Use loop instead of switch case.
---
binutils/elfcomm.c | 124 ++++++---------------------------------------
1 file changed, 16 insertions(+), 108 deletions(-)
Comments
On 06.07.2026 18:59, Julien Thillard wrote:
> Switch cases are used to read a value with a given size, but
> these functions could be rewritten using a small loop instead,
> avoiding code repetition.
> This is already done in `byte_put_*_endian` functions.
>
> binutils:
>
> * elfcomm.c: Use loop instead of switch case.
While the change looks technically correct (apart from ...
> --- a/binutils/elfcomm.c
> +++ b/binutils/elfcomm.c
> @@ -114,127 +114,35 @@ uint64_t (*byte_get) (const unsigned char *, unsigned int);
> uint64_t
> byte_get_little_endian (const unsigned char *field, unsigned int size)
> {
> - switch (size)
> - {
> - case 1:
> - return *field;
> -
> - case 2:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8));
> -
> - case 3:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16));
> -
> - case 4:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16)
> - | ((uint64_t) field[3] << 24));
> -
> - case 5:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16)
> - | ((uint64_t) field[3] << 24)
> - | ((uint64_t) field[4] << 32));
> + unsigned int i;
> + uint64_t read = 0;
>
> - case 6:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16)
> - | ((uint64_t) field[3] << 24)
> - | ((uint64_t) field[4] << 32)
> - | ((uint64_t) field[5] << 40));
> -
> - case 7:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16)
> - | ((uint64_t) field[3] << 24)
> - | ((uint64_t) field[4] << 32)
> - | ((uint64_t) field[5] << 40)
> - | ((uint64_t) field[6] << 48));
> -
> - case 8:
> - return ((uint64_t) field[0]
> - | ((uint64_t) field[1] << 8)
> - | ((uint64_t) field[2] << 16)
> - | ((uint64_t) field[3] << 24)
> - | ((uint64_t) field[4] << 32)
> - | ((uint64_t) field[5] << 40)
> - | ((uint64_t) field[6] << 48)
> - | ((uint64_t) field[7] << 56));
> -
> - default:
> + if (size > sizeof (uint64_t))
> + {
> error (_("Unhandled data length: %d\n"), size);
> abort ();
> }
> + for(i = 0; i < size; i++)
... a style issue here [missing blank after "for"] and again in the other
loop), how does generated code change? There may have been a reason things
were coded this way.
There's also a subtle, possibly (but not necessarily) benign change in
behavior: size being 0 previously hit the default: label, while now we'd
silently return 0 in that case (which may be correct in some cases, but
which may not be desired in others).
Maybe this as a compromise, reducing code size while retaining original
properties?
uint64_t
byte_get_little_endian (const unsigned char *field, unsigned int size)
{
uint64_t read = 0;
switch (size)
{
case 8:
read |= (uint64_t) field[7] << 56;
/* Fall through. */
case 7:
read |= (uint64_t) field[6] << 48;
/* Fall through. */
case 6:
read |= (uint64_t) field[5] << 40;
/* Fall through. */
case 5:
read |= (uint64_t) field[4] << 32;
/* Fall through. */
case 4:
read |= (uint64_t) field[3] << 24;
/* Fall through. */
case 3:
read |= (uint64_t) field[2] << 16;
/* Fall through. */
case 2:
read |= (uint64_t) field[1] << 8;
/* Fall through. */
case 1:
return read | *field;
default:
error (_("Unhandled data length: %u\n"), size);
abort ();
}
}
Jan
@@ -114,127 +114,35 @@ uint64_t (*byte_get) (const unsigned char *, unsigned int);
uint64_t
byte_get_little_endian (const unsigned char *field, unsigned int size)
{
- switch (size)
- {
- case 1:
- return *field;
-
- case 2:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8));
-
- case 3:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16));
-
- case 4:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[3] << 24));
-
- case 5:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[3] << 24)
- | ((uint64_t) field[4] << 32));
+ unsigned int i;
+ uint64_t read = 0;
- case 6:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[3] << 24)
- | ((uint64_t) field[4] << 32)
- | ((uint64_t) field[5] << 40));
-
- case 7:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[3] << 24)
- | ((uint64_t) field[4] << 32)
- | ((uint64_t) field[5] << 40)
- | ((uint64_t) field[6] << 48));
-
- case 8:
- return ((uint64_t) field[0]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[3] << 24)
- | ((uint64_t) field[4] << 32)
- | ((uint64_t) field[5] << 40)
- | ((uint64_t) field[6] << 48)
- | ((uint64_t) field[7] << 56));
-
- default:
+ if (size > sizeof (uint64_t))
+ {
error (_("Unhandled data length: %d\n"), size);
abort ();
}
+ for(i = 0; i < size; i++)
+ read |= (uint64_t) field[i] << (i * 8);
+
+ return read;
}
uint64_t
byte_get_big_endian (const unsigned char *field, unsigned int size)
{
- switch (size)
- {
- case 1:
- return *field;
-
- case 2:
- return ((uint64_t) field[1]
- | ((uint64_t) field[0] << 8));
-
- case 3:
- return ((uint64_t) field[2]
- | ((uint64_t) field[1] << 8)
- | ((uint64_t) field[0] << 16));
-
- case 4:
- return ((uint64_t) field[3]
- | ((uint64_t) field[2] << 8)
- | ((uint64_t) field[1] << 16)
- | ((uint64_t) field[0] << 24));
-
- case 5:
- return ((uint64_t) field[4]
- | ((uint64_t) field[3] << 8)
- | ((uint64_t) field[2] << 16)
- | ((uint64_t) field[1] << 24)
- | ((uint64_t) field[0] << 32));
+ unsigned int i;
+ uint64_t read = 0;
- case 6:
- return ((uint64_t) field[5]
- | ((uint64_t) field[4] << 8)
- | ((uint64_t) field[3] << 16)
- | ((uint64_t) field[2] << 24)
- | ((uint64_t) field[1] << 32)
- | ((uint64_t) field[0] << 40));
-
- case 7:
- return ((uint64_t) field[6]
- | ((uint64_t) field[5] << 8)
- | ((uint64_t) field[4] << 16)
- | ((uint64_t) field[3] << 24)
- | ((uint64_t) field[2] << 32)
- | ((uint64_t) field[1] << 40)
- | ((uint64_t) field[0] << 48));
-
- case 8:
- return ((uint64_t) field[7]
- | ((uint64_t) field[6] << 8)
- | ((uint64_t) field[5] << 16)
- | ((uint64_t) field[4] << 24)
- | ((uint64_t) field[3] << 32)
- | ((uint64_t) field[2] << 40)
- | ((uint64_t) field[1] << 48)
- | ((uint64_t) field[0] << 56));
-
- default:
+ if (size > sizeof (uint64_t))
+ {
error (_("Unhandled data length: %d\n"), size);
abort ();
}
+ for(i = 0; i < size; i++)
+ read |= (uint64_t) field[size - i - 1] << (i * 8);
+
+ return read;
}
uint64_t