Re: [PATCH v2] objcopy: Add elf header e_flags option in objcopy.

[liuzhensong <liuzhensong@loongson.cn>]

On 2022/4/15 下午6:06, WANG Xuerui wrote:
> Hi,
>
> On 4/15/22 17:41, liuzhensong wrote:
>> Specify flags when copying binary to elf file.
>> This flags will be written to the e_flags of the elf header.
>>
>> usage:
>>    objcopy -I binary -O target --alt-elf-eflags=val bin_file elf_file
>>
>> binutils/
>>    * objcopy.c
>>    * doc/binutils.texi:Add the new objcopy option.
>> ---
>>   binutils/doc/binutils.texi |  7 +++++++
>>   binutils/objcopy.c         | 16 ++++++++++++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
>> index 2c234c682aa..b36aad05c98 100644
>> --- a/binutils/doc/binutils.texi
>> +++ b/binutils/doc/binutils.texi
>> @@ -1302,6 +1302,7 @@ objcopy [@option{-F} 
>> @var{bfdname}|@option{--target=}@var{bfdname}]
>>           [@option{--weaken-symbols=}@var{filename}]
>>           [@option{--add-symbol} 
>> @var{name}=[@var{section}:]@var{value}[,@var{flags}]]
>>           [@option{--alt-machine-code=}@var{index}]
>> +        [@option{--alt-elf-eflags=}@var{value}]
>>           [@option{--prefix-symbols=}@var{string}]
>>           [@option{--prefix-sections=}@var{string}]
>>           [@option{--prefix-alloc-sections=}@var{string}]
>> @@ -1942,6 +1943,12 @@ being used.  For ELF based architectures if 
>> the @var{index}
>>   alternative does not exist then the value is treated as an absolute
>>   number to be stored in the e_machine field of the ELF header.
>>   +@item --alt-elf-eflags=@var{value}
>> +If the output file has alternate ELF header e_flags, use the 
>> @var{vaule}
>
> Typo; "value".
>
>> +instead of the default one.  This is useful in case objcopy a binary to
>> +an ELF to specify the e_flags. The value is treated as an absolute
>> +number to be stored in the e_flags field of the ELF header.
>
> Actually the whole description seems inaccurate (which field is the 
> alternate to e_flags? what's e_flags's "default" value?), and the 
> English is broken.
>
> By reading the implementation it's clear the value specified here 
> would get into any output file that is ELF. So no need to call it 
> "alt"; a name like "--elf-eflags" would be enough.
>
> Something like this would be better IMO:
>
> "Meaningful only for ELF output.  Use @var{value} as e_flags of the 
> output.  This is useful for changing the e_flags of an existing 
> binary.  The @var{value} is treated as an absolute number to be stored 
> in the e_flags field of the ELF header."
>
>> +
>>   @item --writable-text
>>   Mark the output text as writable.  This option isn't meaningful for 
>> all
>>   object file formats.
>> diff --git a/binutils/objcopy.c b/binutils/objcopy.c
>> index 6fb31c8cac7..032caf565d1 100644
>> --- a/binutils/objcopy.c
>> +++ b/binutils/objcopy.c
>> @@ -303,6 +303,10 @@ enum long_section_name_handling
>>      This is also the only behaviour for 'strip'.  */
>>   static enum long_section_name_handling long_section_names = KEEP;
>>   +/* Use alternative elf header e_flags?  */
>> +static bool alt_elf_eflags_set = false;
>> +static unsigned int alt_elf_eflags = 0;
>> +
>>   /* 150 isn't special; it's just an arbitrary non-ASCII char value.  */
>>   enum command_line_switch
>>   {
>> @@ -310,6 +314,7 @@ enum command_line_switch
>>     OPTION_ADD_GNU_DEBUGLINK,
>>     OPTION_ADD_SYMBOL,
>>     OPTION_ALT_MACH_CODE,
>> +  OPTION_ALT_ELF_EFLAGS,
>>     OPTION_CHANGE_ADDRESSES,
>>     OPTION_CHANGE_LEADING_CHAR,
>>     OPTION_CHANGE_SECTION_ADDRESS,
>> @@ -427,6 +432,7 @@ static struct option copy_options[] =
>>     {"adjust-vma", required_argument, 0, OPTION_CHANGE_ADDRESSES},
>>     {"adjust-warnings", no_argument, 0, OPTION_CHANGE_WARNINGS},
>>     {"alt-machine-code", required_argument, 0, OPTION_ALT_MACH_CODE},
>> +  {"alt-elf-eflags", required_argument, 0, OPTION_ALT_ELF_EFLAGS},
>>     {"binary-architecture", required_argument, 0, 'B'},
>>     {"byte", required_argument, 0, 'b'},
>>     {"change-addresses", required_argument, 0, OPTION_CHANGE_ADDRESSES},
>> @@ -660,6 +666,7 @@ copy_usage (FILE *stream, int exit_status)
>>        --weaken-symbols <file>       -W for all symbols listed in 
>> <file>\n\
>>        --add-symbol <name>=[<section>:]<value>[,<flags>] Add a symbol\n\
>>        --alt-machine-code <index>    Use the target's <index>'th 
>> alternative machine\n\
>> +     --alt-elf-eflags=<value>      Use the alternative elf header 
>> e_flags\n\
>>        --writable-text               Mark the output text as writable\n\
>>        --readonly-text               Make the output text write 
>> protected\n\
>>        --pure                        Mark the output file as demand 
>> paged\n\
>> @@ -3496,6 +3503,10 @@ copy_object (bfd *ibfd, bfd *obfd, const 
>> bfd_arch_info_type *input_arch)
>>       }
>>       }
>>   +  if (bfd_get_flavour (obfd) == bfd_target_elf_flavour
>> +      && alt_elf_eflags_set)
>> +    elf_elfheader (obfd)->e_flags = alt_elf_eflags;
>> +
>>     return true;
>>   }
>>   @@ -5863,6 +5874,11 @@ copy_main (int argc, char *argv[])
>>           fatal (_("verilog data width must be at least 1 byte"));
>>         break;
>>   +    case OPTION_ALT_ELF_EFLAGS:
>> +      alt_elf_eflags_set = true;
>> +      alt_elf_eflags = parse_vma (optarg, "--alt-elf-eflags");
>> +      break;
>> +
>>       case 0:
>>         /* We've been given a long option.  */
>>         break;


Thank you for the review, it looks more appropriate.

I will send a v3 with the changes.

--
liuzhensong