Re: [PATCH RESEND] elfedit: add support for editing e_flags

[Jan Beulich <jbeulich@suse.com>]

On 02.03.2023 09:14, WANG Xuerui wrote:
> @@ -5376,6 +5378,15 @@ isn't specified, it will match any ELF ABIVERSIONs.
>  Change the ELF ABIVERSION in the ELF header to @var{version}.
>  @var{version} must be between 0 and 255.
>  
> +@item --input-flags=@var{flags}
> +Set the matching input ELF file @var{e_flags} value to @var{flags}.
> +@var{flags} must be an unsigned 32-bit integer.  If @option{--input-flags}
> +isn't specified, it will match any e_flags value.

This doesn't really explain what the behavior is; I had to look at the code
to see what's intended. However, ...

> +@item --output-flags=@var{flags}
> +Change the @var{e_flags} field in the ELF header to @var{flags}.
> +@var{version} must be an unsigned 32-bit integer.

... instead of this pair of options, for anything flags-ish wouldn't it be
more convenient to have a way to merely specify a flags-to-clear and a flags-
to-set value? That would imo also eliminate the need for checking that the
input object's flags match a certain value.

> @@ -394,7 +398,16 @@ update_elf_header (const char *file_name, FILE *file)
>        return 0;
>      }
>  
> -  /* Update e_machine, e_type and EI_OSABI.  */
> +  /* Skip if e_flags doesn't match. */
> +  if (check_elf_flags && elf_header.e_flags != input_elf_flags)
> +    {
> +      error
> +	(_("%s: Unmatched e_flags: 0x%lx is not 0x%x\n"),

May I suggest to use %#x (and alike) wherever suitable? It's a shorter
string literal and doesn't needlessly clutter the value of 0 with a 0x
prefix.

> @@ -1062,6 +1088,28 @@ main (int argc, char ** argv)
>  	    }
>  	  break;
>  
> +	case OPTION_INPUT_FLAGS:
> +	  check_elf_flags = 1;

Please use bool/true/false for boolean variables.

> +	  tmp = strtoul (optarg, &end, 0);
> +	  if (*end != '\0' || tmp > 0xffffffff)

I'm afraid this may trigger compiler warnings: For one because the constant
isn't flagged as unsigned. And then (for 32-bit targets) for being an
always-true comparison.

> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/elfedit-7.d
> @@ -0,0 +1,10 @@
> +#PROG: elfedit
> +#elfedit: --output-flags 0xfedcba98
> +#source: empty.s
> +#readelf: -h
> +#name: Update ELF header 7 (hexadecimal e_flags value)
> +#target: *-*-linux* *-*-gnu*

Any reason *-*-elf* isn't included here? But I guess you really want to
use [is_elf_format] anyway.

Also neither of the two tests uses the other option. If that's to remain,
I think it wants testing (I realize that doing so may require further
limiting the targets on which such a test can be easily run).

Jan