Re: [patch] Add bfd_target_info function to bfd and unify windres/windmc target information gathering

[Nick Clifton]

Hi Kai,

  [Sorry for the delay in reviewing this patch].

> this patch adds to bfd's targets.c an new function, which is mainly a
> move from code in windres/windmc about target information gathering.
> This function is mainly the same as bfd_find_target function, but it
> allows to query optional for endianess, underscoring-character, and
> architecture for the target, too.

Hmm, I am not sure about this patch.  I can appreciate that you want
to eliminate the duplication of some code, but I think that it could
be done in a slightly cleaner way.  In particular I think it might be
simpler to avoid having your new function call bfd_find_target(), but
instead pass to it the bfd_target vector that is returned by
bfd_find_target().  Then it can be used for any target vector.

As for the patch itself there are a few issues:


  +/* Helper function for bfd_target_info.  */

It would be nice to say what this helper function does.


  +static int

This function should return a bfd_boolean not an int.


  +  while (*arch != NULL)

Bug - the code should check for a NULL value in the arch parameter
before using it.  In theory if bfd_arch_list() returns NULL in
bfd_target_info then _bfd_find_arch_match() could be called with a
NULL arch parameter.


  +      char end_ch = (in_a ? in_a[strlen(tname)] : 0);

Space between strlen and (tname).


  + const bfd_target * bfd_target_info (const char *target_name, bfd *abfd,
  +    int *is_bigendian, int *underscoring,
  +    const char **def_target_arch);

Assuming that you agree with my suggestion above then I would consider
renaming this function to something like bfd_get_target_info() and
have it return the default target architecture.

Also the is_bigendian field really ought to be a bfd_boolean * rather
than an int *.  If an invalid target is passed to this new function
then it should return NULL and in that case none of the other
returnable parameters should be used.


  +      const char ** arches = bfd_arch_list();

Space between bfd_arch_list and ().

Cheers
  Nick

[Kai Tietz-2]

Hi Nick,

2009/11/9 Nick Clifton <nickc@...>:

> Hi Kai,
>
>  [Sorry for the delay in reviewing this patch].
>
>> this patch adds to bfd's targets.c an new function, which is mainly a
>> move from code in windres/windmc about target information gathering.
>> This function is mainly the same as bfd_find_target function, but it
>> allows to query optional for endianess, underscoring-character, and
>> architecture for the target, too.
>
> Hmm, I am not sure about this patch.  I can appreciate that you want
> to eliminate the duplication of some code, but I think that it could
> be done in a slightly cleaner way.  In particular I think it might be
> simpler to avoid having your new function call bfd_find_target(), but
> instead pass to it the bfd_target vector that is returned by
> bfd_find_target().  Then it can be used for any target vector.

Well, this is maybe better. I just wanted to keep the function as easy
as possible, but you are right that possibly the use of bfd_target
vector as input argument makes sense here.

> As for the patch itself there are a few issues:
>
>
>  +/* Helper function for bfd_target_info.  */
>
> It would be nice to say what this helper function does.

Well, I will add here some more comments. Mainly it is present to
support internal target names, which are triplets and not tuples (as
for some wince arm targets introduced). So architecture can be found
proper.

>  +static int
>
> This function should return a bfd_boolean not an int.

Ok.

>
>  +  while (*arch != NULL)
>
> Bug - the code should check for a NULL value in the arch parameter
> before using it.  In theory if bfd_arch_list() returns NULL in
> bfd_target_info then _bfd_find_arch_match() could be called with a
> NULL arch parameter.
Well, is this likely? But you are right, I'll add a check here.

>
>  +      char end_ch = (in_a ? in_a[strlen(tname)] : 0);
>
> Space between strlen and (tname).
>
>
>  +     const bfd_target * bfd_target_info (const char *target_name, bfd *abfd,
>  +                                         int *is_bigendian, int *underscoring,
>  +                                         const char **def_target_arch);
>
> Assuming that you agree with my suggestion above then I would consider
> renaming this function to something like bfd_get_target_info() and
> have it return the default target architecture.

Hmm, exactly this I wanted to avoid. The architecture detection is a
bit costy, so I wanted to have this check optional.

> Also the is_bigendian field really ought to be a bfd_boolean * rather
> than an int *.  If an invalid target is passed to this new function
> then it should return NULL and in that case none of the other
> returnable parameters should be used.
Hmm, about the failure return NULL I agree here. To change arguments
to be bfd_boolean * instead could be right. The idea here was to have
a tri-state, but well, possibly this isn't necessary.

>
>  +      const char ** arches = bfd_arch_list();
>
> Space between bfd_arch_list and ().
>
> Cheers
>  Nick
>

Cheers,
Kai

--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[Nick Clifton]

Hi Kai,

>>  +     const bfd_target * bfd_target_info (const char *target_name, bfd *abfd,
>>  +                                         int *is_bigendian, int *underscoring,
>>  +                                         const char **def_target_arch);
>>
>> Assuming that you agree with my suggestion above then I would consider
>> renaming this function to something like bfd_get_target_info() and
>> have it return the default target architecture.
> Hmm, exactly this I wanted to avoid. The architecture detection is a
> bit costy, so I wanted to have this check optional.

But the other information returned by this function (endianness, prefix
character) is easily extracted from the target vector.  There is no need
to have a special function to compute it.  So the real value of this new
function is costly architecture detection.

Cheers
   Nick

[Kai Tietz-2]

2009/11/11 Nick Clifton <nickc@...>:

> Hi Kai,
>
>>>  +     const bfd_target * bfd_target_info (const char *target_name, bfd
>>> *abfd,
>>>  +                                         int *is_bigendian, int
>>> *underscoring,
>>>  +                                         const char **def_target_arch);
>>>
>>> Assuming that you agree with my suggestion above then I would consider
>>> renaming this function to something like bfd_get_target_info() and
>>> have it return the default target architecture.
>>
>> Hmm, exactly this I wanted to avoid. The architecture detection is a
>> bit costy, so I wanted to have this check optional.
>
> But the other information returned by this function (endianness, prefix
> character) is easily extracted from the target vector.  There is no need to
> have a special function to compute it.  So the real value of this new
> function is costly architecture detection.
>
> Cheers
>  Nick
>
>
>

Well, this was the reason, why I put the target_name check within this
function. I wanted to avoid multiple patterns of the form

bfd_target *t = bfd_find_target (my_target, NULL);
if (!t) error out
my_store_symbol_prefix = t->symbol_prefix;

 ....

As most of the bfd helper macros here depend on an existing bfd *. But
a bfd * is at some points not present (eg windres tool and windmc), so
a function to gather those information by a common function is better
then re-implementing again and again.

Cheers,
Kai
--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[Nick Clifton]

Hi Kai,

> Well, this was the reason, why I put the target_name check within this
> function. I wanted to avoid multiple patterns of the form
>
> bfd_target *t = bfd_find_target (my_target, NULL);
> if (!t) error out
> my_store_symbol_prefix = t->symbol_prefix;
>
>  ....
>
> As most of the bfd helper macros here depend on an existing bfd *. But
> a bfd * is at some points not present (eg windres tool and windmc), so
> a function to gather those information by a common function is better
> then re-implementing again and again.

OK, that is fair.  In which case lets go with your original plan for the
function.

Cheers
   Nick

[Kai Tietz-2]

Hi Nick,

2009/11/12 Nick Clifton <nickc@...>:

> Hi Kai,
>
>> Well, this was the reason, why I put the target_name check within this
>> function. I wanted to avoid multiple patterns of the form
>>
>> bfd_target *t = bfd_find_target (my_target, NULL);
>> if (!t) error out
>> my_store_symbol_prefix = t->symbol_prefix;
>>
>>  ....
>>
>> As most of the bfd helper macros here depend on an existing bfd *. But
>> a bfd * is at some points not present (eg windres tool and windmc), so
>> a function to gather those information by a common function is better
>> then re-implementing again and again.
>
> OK, that is fair.  In which case lets go with your original plan for the
> function.
>
> Cheers
>  Nick

Ok, here is the updated patch.

ChangeLog for bfd

2009-11-14  Kai Tietz  <kai.tietz@...>

        * targets.c (bfd_get_target_info): New function.
        (_bfd_find_arch_match): New function.
        * bfd-in2.h: Regenerated.

ChangeLog for binutils

2009-11-14  Kai Tietz  <kai.tietz@...>

        * windmc.c (set_endianess): Use bfd_get_target_info.
        * windres.c (set_endianess): Likewise.
        (find_arch_match): Removed.

Tested for i686-pc-mingw32, i686-pc-cygwin, and x86_64-pc-mingw32. Ok for apply?

Cheers,
Kai

--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[Dave Korn-6]

Kai Tietz wrote:

> Tested for i686-pc-mingw32, i686-pc-cygwin, and x86_64-pc-mingw32. Ok for apply?

  No objections from the PE point of view (disclaimer: didn't build or test
the patch myself), but wait for Nick to OK the global changes to bfd.  Few
formatting nits:

> +const bfd_target * bfd_get_target_info (const char *target_name, bfd *abfd,

  Space before the * but not after, like bfd_find_target above.

> +_bfd_find_arch_match (const char *tname,const char **arch,

  Space needed before second 'const'.

> + If @var{is_bigendian} is not <<NULL>>, then set this value to target's
> + endian mode. One for big-endian, zero for little-endian, and -1 for
> + invalid target.

  This is not what the code actually does, since is_bigendian is a bfd_boolean
rather than an int type.  I'd suggest you just change the documentation to say
that it defaults to false for invalid target rather than change the type,
there are enough other ways for the caller to know that the target was not valid.

+      /* Make sure we dectect architecture names

  Typo "dectect" -> "detect".

    cheers,
      DaveK

[Kai Tietz-2]

Dave,

thanks, here is updated version.

Cheers,
Kai

--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[Dave Korn-6]

Kai Tietz wrote:
> Dave,
>
> thanks, here is updated version.

  You still missed one superfluous space:

>  const bfd_target *bfd_find_target (const char *target_name, bfd *abfd);
>  
> +const bfd_target * bfd_get_target_info (const char *target_name,
                     ^
  The asterisk always goes hard up against the symbol, because that's how it
binds (the canonical example being "char* x,y;" vs. "char *x,y;").  Everything
else looked good.

    cheers,
      DaveK

[Kai Tietz-2]

2009/11/14 Dave Korn <dave.korn.cygwin@...>:

> Kai Tietz wrote:
>> Dave,
>>
>> thanks, here is updated version.
>
>  You still missed one superfluous space:
>
>>  const bfd_target *bfd_find_target (const char *target_name, bfd *abfd);
>>
>> +const bfd_target * bfd_get_target_info (const char *target_name,
>                     ^
>  The asterisk always goes hard up against the symbol, because that's how it
> binds (the canonical example being "char* x,y;" vs. "char *x,y;").  Everything
> else looked good.
>
>    cheers,
>      DaveK
>

Ah, the prototype in comment, which gets exported into bfd-in2.h I've missed.

Here is updated patch.

Ok for apply?

Cheers,
Kai

--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[Nick Clifton]

Hi Kai,

> Ok for apply?

Approved - please apply - although I would ask for one small formatting
change:

   +bfd_get_target_info (const char *target_name, bfd *abfd,
   +     bfd_boolean *is_bigendian,
   +     int *underscoring, const char **def_target_arch)
   +{
   +  const bfd_target *target_vec;
   +  if (is_bigendian)
   +    *is_bigendian = FALSE;

Please could you place a blank line after the declaration of target_vec
and before the start of the if-statement.

Cheers
   Nick

[Kai Tietz-2]

2009/11/16 Nick Clifton <nickc@...>:

> Hi Kai,
>
>> Ok for apply?
>
> Approved - please apply - although I would ask for one small formatting
> change:
>
>  +bfd_get_target_info (const char *target_name, bfd *abfd,
>  +                  bfd_boolean *is_bigendian,
>  +                  int *underscoring, const char **def_target_arch)
>  +{
>  +  const bfd_target *target_vec;
>  +  if (is_bigendian)
>  +    *is_bigendian = FALSE;
>
> Please could you place a blank line after the declaration of target_vec and
> before the start of the if-statement.

Hello Nick,

ok, done. And applied.

Thanks,
Kai


--
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination