This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH, binutils/ARM] Reduce parameter list in bfd_elf32_arm_target_relocs
- From: "Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>
- To: Thomas Preudhomme <thomas dot preudhomme at foss dot arm dot com>, binutils at sourceware dot org, Nick Clifton <nickc at redhat dot com>, Alan Modra <amodra at gmail dot com>
- Date: Fri, 26 Aug 2016 13:38:22 +0100
- Subject: Re: [PATCH, binutils/ARM] Reduce parameter list in bfd_elf32_arm_target_relocs
- Authentication-results: sourceware.org; auth=none
- References: <c3158cf0-d300-d866-146c-58fc6570c2ea@foss.arm.com>
On 26/08/16 12:19, Thomas Preudhomme wrote:
> Hi,
>
> As pointed out by Richard Earnshaw in [1], bfd_elf32_arm_target_relocs
> now takes 15 parameters. This commit creates a new structure to contain
> all of them but the bfd and bfd_link_info values and adapt the function
> to take that structure as parameter instead. While doing so, the
> function is renamed to bfd_elf32_arm_target_params to reflect that the
> settings affect more than just relocation.
>
> [1] https://sourceware.org/ml/binutils/2016-07/msg00099.html
>
> ChangeLog entries are as follow:
>
> *** bfd/ChangeLog ***
>
> 2016-07-08 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * bfd-in.h (struct elf32_arm_params): Define.
> (bfd_elf32_arm_set_target_relocs): Rename into ...
> (bfd_elf32_arm_set_target_params): This. Use a struct
> elf32_arm_params to pass all parameters but the bfd and
> bfd_link_info.
> * bfd-in2.h: Regenerate.
> * elf32-arm.c (bfd_elf32_arm_set_target_relocs): Rename into ...
> (bfd_elf32_arm_set_target_params): This. Pass all values via a
> struct
> elf32_arm_params rather than as individual parameters.
>
>
> *** ld/ChangeLog ***
>
> 2016-08-18 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * emultempl/armelf.em (params): New static variable.
> (thumb_entry_symbol, byteswap_code, target1_is_rel, target2_type,
> fix_v4bx, use_blx, vfp11_denorm_fix, stm32l4xx_fix, fix_cortex_a8,
> no_enum_size_warning, no_wchar_size_warning, pic_veneer,
> merge_exidx_entries, fix_arm1176, cmse_implib): move as part of the
> above new structure.
> (arm_elf_before_allocation): Access static variable from the params
> structure.
> (gld${EMULATION_NAME}_finish): Likewise.
> (arm_elf_create_output_section_statements): Likewise and pass the
> address of that structure to bfd_elf32_arm_set_target_relocs
> instead
> of the static variables.
> (PARSE_AND_LIST_ARGS_CASES): Access static variable from the params
> structure.
>
>
> binutils build correctly after the change and testsuite shows no
> regression for arm-none-eabi targets.
>
> Is this ok for master branch?
I think this is much better. OK.
R.
>
> Best regards,
>
> Thomas
>
> reduce_arm_target_relocs_param_list.patch
>
>
> diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
> index 8e50bc5e5f379e3d7bb45f6221d0fe809691550f..fa3da8327c5cf6f7801564564713a4d02127f6e1 100644
> --- a/bfd/bfd-in.h
> +++ b/bfd/bfd-in.h
> @@ -900,9 +900,27 @@ extern bfd_boolean bfd_elf32_arm_allocate_interworking_sections
> extern bfd_boolean bfd_elf32_arm_process_before_allocation
> (bfd *, struct bfd_link_info *);
>
> -void bfd_elf32_arm_set_target_relocs
> - (bfd *, struct bfd_link_info *, int, char *, int, int, bfd_arm_vfp11_fix,
> - bfd_arm_stm32l4xx_fix, int, int, int, int, int, int, bfd *);
> +struct elf32_arm_params {
> + char *thumb_entry_symbol;
> + int byteswap_code;
> + int target1_is_rel;
> + char * target2_type;
> + int fix_v4bx;
> + int use_blx;
> + bfd_arm_vfp11_fix vfp11_denorm_fix;
> + bfd_arm_stm32l4xx_fix stm32l4xx_fix;
> + int no_enum_size_warning;
> + int no_wchar_size_warning;
> + int pic_veneer;
> + int fix_cortex_a8;
> + int fix_arm1176;
> + int merge_exidx_entries;
> + int cmse_implib;
> + bfd *in_implib_bfd;
> +};
> +
> +void bfd_elf32_arm_set_target_params
> + (bfd *, struct bfd_link_info *, struct elf32_arm_params *);
>
> extern bfd_boolean bfd_elf32_arm_get_bfd_for_interworking
> (bfd *, struct bfd_link_info *);
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index ffa6c57f43289a756b850c71d0f3720501554cdf..f8188ab8c8beec793b69c4f6bf9a933dc15bf8ba 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -907,9 +907,27 @@ extern bfd_boolean bfd_elf32_arm_allocate_interworking_sections
> extern bfd_boolean bfd_elf32_arm_process_before_allocation
> (bfd *, struct bfd_link_info *);
>
> -void bfd_elf32_arm_set_target_relocs
> - (bfd *, struct bfd_link_info *, int, char *, int, int, bfd_arm_vfp11_fix,
> - bfd_arm_stm32l4xx_fix, int, int, int, int, int, int, bfd *);
> +struct elf32_arm_params {
> + char *thumb_entry_symbol;
> + int byteswap_code;
> + int target1_is_rel;
> + char * target2_type;
> + int fix_v4bx;
> + int use_blx;
> + bfd_arm_vfp11_fix vfp11_denorm_fix;
> + bfd_arm_stm32l4xx_fix stm32l4xx_fix;
> + int no_enum_size_warning;
> + int no_wchar_size_warning;
> + int pic_veneer;
> + int fix_cortex_a8;
> + int fix_arm1176;
> + int merge_exidx_entries;
> + int cmse_implib;
> + bfd *in_implib_bfd;
> +};
> +
> +void bfd_elf32_arm_set_target_params
> + (bfd *, struct bfd_link_info *, struct elf32_arm_params *);
>
> extern bfd_boolean bfd_elf32_arm_get_bfd_for_interworking
> (bfd *, struct bfd_link_info *);
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index b942d1a8e93628dcdbd6343ca35c49913e748867..21ce494e5ccc99b936bdf123967d3c15c37e10c3 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -8674,18 +8674,9 @@ error_return:
> /* Set target relocation values needed during linking. */
>
> void
> -bfd_elf32_arm_set_target_relocs (struct bfd *output_bfd,
> +bfd_elf32_arm_set_target_params (struct bfd *output_bfd,
> struct bfd_link_info *link_info,
> - int target1_is_rel,
> - char * target2_type,
> - int fix_v4bx,
> - int use_blx,
> - bfd_arm_vfp11_fix vfp11_fix,
> - bfd_arm_stm32l4xx_fix stm32l4xx_fix,
> - int no_enum_warn, int no_wchar_warn,
> - int pic_veneer, int fix_cortex_a8,
> - int fix_arm1176, int cmse_implib,
> - bfd *in_implib_bfd)
> + struct elf32_arm_params *params)
> {
> struct elf32_arm_link_hash_table *globals;
>
> @@ -8693,31 +8684,33 @@ bfd_elf32_arm_set_target_relocs (struct bfd *output_bfd,
> if (globals == NULL)
> return;
>
> - globals->target1_is_rel = target1_is_rel;
> - if (strcmp (target2_type, "rel") == 0)
> + globals->target1_is_rel = params->target1_is_rel;
> + if (strcmp (params->target2_type, "rel") == 0)
> globals->target2_reloc = R_ARM_REL32;
> - else if (strcmp (target2_type, "abs") == 0)
> + else if (strcmp (params->target2_type, "abs") == 0)
> globals->target2_reloc = R_ARM_ABS32;
> - else if (strcmp (target2_type, "got-rel") == 0)
> + else if (strcmp (params->target2_type, "got-rel") == 0)
> globals->target2_reloc = R_ARM_GOT_PREL;
> else
> {
> _bfd_error_handler (_("Invalid TARGET2 relocation type '%s'."),
> - target2_type);
> + params->target2_type);
> }
> - globals->fix_v4bx = fix_v4bx;
> - globals->use_blx |= use_blx;
> - globals->vfp11_fix = vfp11_fix;
> - globals->stm32l4xx_fix = stm32l4xx_fix;
> - globals->pic_veneer = pic_veneer;
> - globals->fix_cortex_a8 = fix_cortex_a8;
> - globals->fix_arm1176 = fix_arm1176;
> - globals->cmse_implib = cmse_implib;
> - globals->in_implib_bfd = in_implib_bfd;
> + globals->fix_v4bx = params->fix_v4bx;
> + globals->use_blx |= params->use_blx;
> + globals->vfp11_fix = params->vfp11_denorm_fix;
> + globals->stm32l4xx_fix = params->stm32l4xx_fix;
> + globals->pic_veneer = params->pic_veneer;
> + globals->fix_cortex_a8 = params->fix_cortex_a8;
> + globals->fix_arm1176 = params->fix_arm1176;
> + globals->cmse_implib = params->cmse_implib;
> + globals->in_implib_bfd = params->in_implib_bfd;
>
> BFD_ASSERT (is_arm_elf (output_bfd));
> - elf_arm_tdata (output_bfd)->no_enum_size_warning = no_enum_warn;
> - elf_arm_tdata (output_bfd)->no_wchar_size_warning = no_wchar_warn;
> + elf_arm_tdata (output_bfd)->no_enum_size_warning
> + = params->no_enum_size_warning;
> + elf_arm_tdata (output_bfd)->no_wchar_size_warning
> + = params->no_wchar_size_warning;
> }
>
> /* Replace the target offset of a Thumb bl or b.w instruction. */
> diff --git a/ld/emultempl/armelf.em b/ld/emultempl/armelf.em
> index 2346bb42444895605d10b7b7e21cca2dcaa38700..02f6f9d88ffd49d66a12b0c74883e55621c3f7fc 100644
> --- a/ld/emultempl/armelf.em
> +++ b/ld/emultempl/armelf.em
> @@ -28,21 +28,25 @@ fragment <<EOF
> #include "ldctor.h"
> #include "elf/arm.h"
>
> -static char * thumb_entry_symbol = NULL;
> -static int byteswap_code = 0;
> -static int target1_is_rel = 0${TARGET1_IS_REL};
> -static char * target2_type = "${TARGET2_TYPE}";
> -static int fix_v4bx = 0;
> -static int use_blx = 0;
> -static bfd_arm_vfp11_fix vfp11_denorm_fix = BFD_ARM_VFP11_FIX_DEFAULT;
> -static bfd_arm_stm32l4xx_fix stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_NONE;
> -static int fix_cortex_a8 = -1;
> -static int no_enum_size_warning = 0;
> -static int no_wchar_size_warning = 0;
> -static int pic_veneer = 0;
> -static int merge_exidx_entries = -1;
> -static int fix_arm1176 = 1;
> -static int cmse_implib = 0;
> +static struct elf32_arm_params params =
> +{
> + NULL, /* thumb_entry_symbol */
> + 0, /* byteswap_code */
> + 0${TARGET1_IS_REL}, /* target1_is_rel */
> + "${TARGET2_TYPE}", /* target2_type */
> + 0, /* fix_v4bx */
> + 0, /* use_blx */
> + BFD_ARM_VFP11_FIX_DEFAULT, /* vfp11_denorm_fix */
> + BFD_ARM_STM32L4XX_FIX_NONE, /* stm32l4xx_fix */
> + 0, /* no_enum_size_warning */
> + 0, /* no_wchar_size_warning */
> + 0, /* pic_veneer */
> + -1, /* fix_cortex_a8 */
> + 1, /* fix_arm1176 */
> + -1, /* merge_exidx_entries */
> + 0, /* cmse_implib */
> + NULL /* in_implib_bfd */
> +};
> static char *in_implib_filename = NULL;
>
> static void
> @@ -72,7 +76,7 @@ gld${EMULATION_NAME}_set_symbols (void)
> static void
> arm_elf_before_allocation (void)
> {
> - bfd_elf32_arm_set_byteswap_code (&link_info, byteswap_code);
> + bfd_elf32_arm_set_byteswap_code (&link_info, params.byteswap_code);
>
> /* Choose type of VFP11 erratum fix, or warn if specified fix is unnecessary
> due to architecture version. */
> @@ -357,7 +361,7 @@ gld${EMULATION_NAME}_after_allocation (void)
> qsort (sec_list, sec_count, sizeof (asection *), &compare_output_sec_vma);
>
> if (elf32_arm_fix_exidx_coverage (sec_list, sec_count, &link_info,
> - merge_exidx_entries))
> + params.merge_exidx_entries))
> need_laying_out = 1;
>
> free (sec_list);
> @@ -438,9 +442,9 @@ gld${EMULATION_NAME}_finish (void)
>
> finish_default ();
>
> - if (thumb_entry_symbol)
> + if (params.thumb_entry_symbol)
> {
> - h = bfd_link_hash_lookup (link_info.hash, thumb_entry_symbol,
> + h = bfd_link_hash_lookup (link_info.hash, params.thumb_entry_symbol,
> FALSE, FALSE, TRUE);
> }
> else
> @@ -483,15 +487,15 @@ gld${EMULATION_NAME}_finish (void)
>
> sprintf_vma (buffer + 2, val);
>
> - if (thumb_entry_symbol != NULL && entry_symbol.name != NULL
> + if (params.thumb_entry_symbol != NULL && entry_symbol.name != NULL
> && entry_from_cmdline)
> einfo (_("%P: warning: '--thumb-entry %s' is overriding '-e %s'\n"),
> - thumb_entry_symbol, entry_symbol.name);
> + params.thumb_entry_symbol, entry_symbol.name);
> entry_symbol.name = buffer;
> }
> else
> einfo (_("%P: warning: cannot find thumb start symbol %s\n"),
> - thumb_entry_symbol);
> + params.thumb_entry_symbol);
> }
>
> /* This is a convenient point to tell BFD about target specific flags.
> @@ -499,8 +503,6 @@ gld${EMULATION_NAME}_finish (void)
> static void
> arm_elf_create_output_section_statements (void)
> {
> - bfd *in_implib_bfd;
> -
> if (strstr (bfd_get_target (link_info.output_bfd), "arm") == NULL)
> {
> /* The arm backend needs special fields in the output hash structure.
> @@ -513,26 +515,17 @@ arm_elf_create_output_section_statements (void)
>
> if (in_implib_filename)
> {
> - in_implib_bfd = bfd_openr (in_implib_filename,
> - bfd_get_target (link_info.output_bfd));
> + params.in_implib_bfd = bfd_openr (in_implib_filename,
> + bfd_get_target (link_info.output_bfd));
>
> - if (in_implib_bfd == NULL)
> + if (params.in_implib_bfd == NULL)
> einfo ("%F%s: Can't open: %E\n", in_implib_filename);
>
> - if (!bfd_check_format (in_implib_bfd, bfd_object))
> + if (!bfd_check_format (params.in_implib_bfd, bfd_object))
> einfo ("%F%s: Not a relocatable file: %E\n", in_implib_filename);
> }
> - else
> - in_implib_bfd = NULL;
>
> - bfd_elf32_arm_set_target_relocs (link_info.output_bfd, &link_info,
> - target1_is_rel,
> - target2_type, fix_v4bx, use_blx,
> - vfp11_denorm_fix, stm32l4xx_fix,
> - no_enum_size_warning,
> - no_wchar_size_warning,
> - pic_veneer, fix_cortex_a8,
> - fix_arm1176, cmse_implib, in_implib_bfd);
> + bfd_elf32_arm_set_target_params (link_info.output_bfd, &link_info, ¶ms);
>
> stub_file = lang_add_input_file ("linker stubs",
> lang_input_file_is_fake_enum,
> @@ -675,71 +668,71 @@ PARSE_AND_LIST_ARGS_CASES='
> break;
>
> case OPTION_THUMB_ENTRY:
> - thumb_entry_symbol = optarg;
> + params.thumb_entry_symbol = optarg;
> break;
>
> case OPTION_BE8:
> - byteswap_code = 1;
> + params.byteswap_code = 1;
> break;
>
> case OPTION_TARGET1_REL:
> - target1_is_rel = 1;
> + params.target1_is_rel = 1;
> break;
>
> case OPTION_TARGET1_ABS:
> - target1_is_rel = 0;
> + params.target1_is_rel = 0;
> break;
>
> case OPTION_TARGET2:
> - target2_type = optarg;
> + params.target2_type = optarg;
> break;
>
> case OPTION_FIX_V4BX:
> - fix_v4bx = 1;
> + params.fix_v4bx = 1;
> break;
>
> case OPTION_FIX_V4BX_INTERWORKING:
> - fix_v4bx = 2;
> + params.fix_v4bx = 2;
> break;
>
> case OPTION_USE_BLX:
> - use_blx = 1;
> + params.use_blx = 1;
> break;
>
> case OPTION_VFP11_DENORM_FIX:
> if (strcmp (optarg, "none") == 0)
> - vfp11_denorm_fix = BFD_ARM_VFP11_FIX_NONE;
> + params.vfp11_denorm_fix = BFD_ARM_VFP11_FIX_NONE;
> else if (strcmp (optarg, "scalar") == 0)
> - vfp11_denorm_fix = BFD_ARM_VFP11_FIX_SCALAR;
> + params.vfp11_denorm_fix = BFD_ARM_VFP11_FIX_SCALAR;
> else if (strcmp (optarg, "vector") == 0)
> - vfp11_denorm_fix = BFD_ARM_VFP11_FIX_VECTOR;
> + params.vfp11_denorm_fix = BFD_ARM_VFP11_FIX_VECTOR;
> else
> einfo (_("Unrecognized VFP11 fix type '\''%s'\''.\n"), optarg);
> break;
>
> case OPTION_STM32L4XX_FIX:
> if (!optarg)
> - stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_DEFAULT;
> + params.stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_DEFAULT;
> else if (strcmp (optarg, "none") == 0)
> - stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_NONE;
> + params.stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_NONE;
> else if (strcmp (optarg, "default") == 0)
> - stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_DEFAULT;
> + params.stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_DEFAULT;
> else if (strcmp (optarg, "all") == 0)
> - stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_ALL;
> + params.stm32l4xx_fix = BFD_ARM_STM32L4XX_FIX_ALL;
> else
> einfo (_("Unrecognized STM32L4XX fix type '\''%s'\''.\n"), optarg);
> break;
>
> case OPTION_NO_ENUM_SIZE_WARNING:
> - no_enum_size_warning = 1;
> + params.no_enum_size_warning = 1;
> break;
>
> case OPTION_NO_WCHAR_SIZE_WARNING:
> - no_wchar_size_warning = 1;
> + params.no_wchar_size_warning = 1;
> break;
>
> case OPTION_PIC_VENEER:
> - pic_veneer = 1;
> + params.pic_veneer = 1;
> break;
>
> case OPTION_STUBGROUP_SIZE:
> @@ -753,23 +746,23 @@ PARSE_AND_LIST_ARGS_CASES='
> break;
>
> case OPTION_FIX_CORTEX_A8:
> - fix_cortex_a8 = 1;
> + params.fix_cortex_a8 = 1;
> break;
>
> case OPTION_NO_FIX_CORTEX_A8:
> - fix_cortex_a8 = 0;
> + params.fix_cortex_a8 = 0;
> break;
>
> case OPTION_NO_MERGE_EXIDX_ENTRIES:
> - merge_exidx_entries = 0;
> + params.merge_exidx_entries = 0;
> break;
>
> case OPTION_FIX_ARM1176:
> - fix_arm1176 = 1;
> + params.fix_arm1176 = 1;
> break;
>
> case OPTION_NO_FIX_ARM1176:
> - fix_arm1176 = 0;
> + params.fix_arm1176 = 0;
> break;
>
> case OPTION_LONG_PLT:
> @@ -777,7 +770,7 @@ PARSE_AND_LIST_ARGS_CASES='
> break;
>
> case OPTION_CMSE_IMPLIB:
> - cmse_implib = 1;
> + params.cmse_implib = 1;
> break;
>
> case OPTION_IN_IMPLIB:
>