[PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Hello!

This patch changes how SET_ARCH_MACH is used. As done currently it calls the
function stored in bfd_target/_bfd_set_arch_mach directly ignoring the bfd
function parameter available at the place the macro is used. In essence this
is bfd_set_arch_mach in disguise. Using a direct call is not wrong (after all
the code was there for almost 20 years and xvec is const), nevertheless using
bfd_set_arch_mach feels to be 'more' correct. The DEFAULT_ARCH is deleted since
its only required iff SET_ARCH_MACH is not defined. I wonder what the set_sizes
method is good for if the method is not called without a bfd_set_arch_mach call?

The additional changes to the local swap_ext_reloc_in function are meant to
align it with the "base" swap_ext_reloc_in function in aoutx.h. About the
r_index casting I am not sure anymore since the commit to aoutx.h talks about
K&R. Maybe the casts in aoutx.h should be removed? The change to r_type
handling is a no-op, since the SHIFT value is 0. However for consistency I
think the code should use the same style as the base code from aoutx.h.


Regards,
Gunther Nikl

2020-04-XX  Gunther Nikl  <gnikl@justmail.de>

	* aout-cris.c (DEFAULT_ARCH): Delete define.
	(MY_set_arch_mach): Likewise.
	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit architecture
	of bfd_arch_cris.
	(swap_ext_reloc_in): Add casts to r_index extraction. Mask valid bits
	of r_type before the shift.

-- cut --
diff --git a/bfd/aout-cris.c b/bfd/aout-cris.c
index 30ab2b5f49..e9ed050a1b 100644
--- a/bfd/aout-cris.c
+++ b/bfd/aout-cris.c
@@ -56,9 +56,6 @@
 #define TARGET_PAGE_SIZE SEGMENT_SIZE
 #define TARGETNAME "a.out-cris"
 
-/* The definition here seems not used; just provided as a convention.  */
-#define DEFAULT_ARCH bfd_arch_cris
-
 /* Do not "beautify" the CONCAT* macro args.  Traditional C will not
    remove whitespace added here, and thus will fail to concatenate
    the tokens.  */
@@ -92,9 +89,8 @@ static bfd_boolean MY (set_sizes) (bfd *);
    through SET_ARCH_MACH.  The default bfd_default_set_arch_mach will
    not call set_sizes.  */
 
-#define MY_set_arch_mach NAME (aout, set_arch_mach)
 #define SET_ARCH_MACH(BFD, EXECP) \
- MY_set_arch_mach (BFD, DEFAULT_ARCH, N_MACHTYPE (EXECP))
+  bfd_set_arch_mach (BFD, bfd_arch_cris, N_MACHTYPE (EXECP))
 
 /* These macros describe the binary layout of the reloc information we
    use in a file.  */
@@ -231,12 +227,12 @@ MY (swap_ext_reloc_in) (bfd *abfd,
   cache_ptr->address = (GET_SWORD (abfd, bytes->r_address));
 
   /* Now the fun stuff.  */
-  r_index =  (bytes->r_index[2] << 16)
-    | (bytes->r_index[1] << 8)
-    |  bytes->r_index[0];
+  r_index =  (((unsigned int) bytes->r_index[2] << 16)
+    | ((unsigned int) bytes->r_index[1] << 8)
+    |  bytes->r_index[0]);
   r_extern = (0 != (bytes->r_type[0] & RELOC_EXT_BITS_EXTERN_LITTLE));
-  r_type = ((bytes->r_type[0]) >> RELOC_EXT_BITS_TYPE_SH_LITTLE)
-    & RELOC_EXT_BITS_TYPE_LITTLE;
+  r_type = ((bytes->r_type[0] & RELOC_EXT_BITS_TYPE_LITTLE)
+    >> RELOC_EXT_BITS_TYPE_SH_LITTLE);
 
   if (r_type > 2)
     {
-- cut --

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Nick Clifton]

Hi Gunther,

> 2020-04-XX  Gunther Nikl  <gnikl@justmail.de>
> 
> 	* aout-cris.c (DEFAULT_ARCH): Delete define.
> 	(MY_set_arch_mach): Likewise.
> 	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit architecture
> 	of bfd_arch_cris.
> 	(swap_ext_reloc_in): Add casts to r_index extraction. Mask valid bits
> 	of r_type before the shift.
 
Approved and applied.

Cheers
  Nick

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Nick Clifton <nickc@redhat.com> wrote:
> 
> > 2020-04-XX  Gunther Nikl  <gnikl@justmail.de>
> > 
> > 	* aout-cris.c (DEFAULT_ARCH): Delete define.
> > 	(MY_set_arch_mach): Likewise.
> > 	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit
> > architecture of bfd_arch_cris.
> > 	(swap_ext_reloc_in): Add casts to r_index extraction. Mask
> > valid bits of r_type before the shift.
>  
> Approved and applied.

Thank you for taking care of the patch!

I see that the patch was committed as-is. I take it that the cast as used
in aoutx.h when building r_index are necessary then? I really don't know
why the casts were added.

What about other (mis)uses of bfd_target vectors? Is bypassing them allowed?
I mean not using a provided BFD_SEND for a vector but calling a function
directly.


Thank you,
Gunther

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Nick Clifton]

Hi Gunther,

> I see that the patch was committed as-is. I take it that the cast as used
> in aoutx.h when building r_index are necessary then?

Well not for me at least.  I suppose that using a different compiler might
theoretically cause problems.  But if necessary we can always add the casts
with another patch.

> What about other (mis)uses of bfd_target vectors? Is bypassing them allowed?

It is discouraged.  It can be done of course, and some of the code is so old
that it was never properly reviewed, but for new code I would always recommend
using the vectors.

Cheers
  Nick

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Hans-Peter Nilsson]

On Mon, 4 May 2020, Nick Clifton via Binutils wrote:
> Hi Gunther,
>
> > 2020-04-XX  Gunther Nikl  <gnikl@justmail.de>
> >
> > 	* aout-cris.c (DEFAULT_ARCH): Delete define.
> > 	(MY_set_arch_mach): Likewise.
> > 	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit architecture
> > 	of bfd_arch_cris.
> > 	(swap_ext_reloc_in): Add casts to r_index extraction. Mask valid bits
> > 	of r_type before the shift.
>
> Approved and applied.

Whoops, I missed this one, posted 2020-04-21.  Thanks for taking
care of it!

Regarding the ensuing discussion, I have no recollection of the
events around the conception :) other than I was possibly
striving for consistency with other similar a.out-related files.

brgds, H-P

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Hans-Peter Nilsson <hp@bitrange.com> wrote:
>
> On Mon, 4 May 2020, Nick Clifton via Binutils wrote:
> > Hi Gunther,
> >
> > > 2020-04-XX  Gunther Nikl  <gnikl@justmail.de>
> > >
> > > 	* aout-cris.c (DEFAULT_ARCH): Delete define.
> > > 	(MY_set_arch_mach): Likewise.
> > > 	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit
> > > architecture of bfd_arch_cris.
> > > 	(swap_ext_reloc_in): Add casts to r_index extraction.
> > > Mask valid bits of r_type before the shift.
> >
> > Approved and applied.
> 
> Whoops, I missed this one, posted 2020-04-21.  Thanks for taking
> care of it!

I sent the mail to the list only since I knew you are reading the list.
I was unsure whether CCing you would be fine. I assumed that you would
notice a CRIS topic since its rather rare.

> Regarding the ensuing discussion, I have no recollection of the
> events around the conception :) other than I was possibly
> striving for consistency with other similar a.out-related files.

IMO, its quite a challenge to understand all these clever macros
filling structure elements, calling functions that have to be named
correctly for other macros. Sometimes a function vector is called
directly by macro (using another macro ;), and sometimes its a real
function which then calls the function pointer. And the list goes on.

BTW, I didn't realize until recently that aoutx.h can be used outside
of aout32.c/aout64.c. However I was surprised to see that aout-cris.c
does include aout32.c directly. I guess that was a deliberate decision?
Anyway including aoutx.h in my custom backend avoided ugly changes to
aoutx.h itself. That was a surprising lesson.

Regards,
Gunther

PS: I hope the mail gets to the list.

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Hans-Peter Nilsson]

On Wed, 6 May 2020, Gunther Nikl wrote:
> Hans-Peter Nilsson <hp@bitrange.com> wrote:
> >
> > On Mon, 4 May 2020, Nick Clifton via Binutils wrote:
> > > Hi Gunther,
> > >
> > > > 2020-04-XX  Gunther Nikl  <gnikl@justmail.de>
> > > >
> > > > 	* aout-cris.c (DEFAULT_ARCH): Delete define.
> > > > 	(MY_set_arch_mach): Likewise.
> > > > 	(SET_ARCH_MACH): Use bfd_set_arch_mach with an explicit
> > > > architecture of bfd_arch_cris.
> > > > 	(swap_ext_reloc_in): Add casts to r_index extraction.
> > > > Mask valid bits of r_type before the shift.
> > >
> > > Approved and applied.
> >
> > Whoops, I missed this one, posted 2020-04-21.  Thanks for taking
> > care of it!
>
> I sent the mail to the list only since I knew you are reading the list.
> I was unsure whether CCing you would be fine.

I don't know of a maintainer that would see that as bad, but
I guess it takes just one such experience...

> I assumed that you would
> notice a CRIS topic since its rather rare.

That's reasonable thinking, mea culpa.

> > Regarding the ensuing discussion, I have no recollection of the
> > events around the conception :) other than I was possibly
> > striving for consistency with other similar a.out-related files.
>
> IMO, its quite a challenge to understand all these clever macros
> filling structure elements, calling functions that have to be named
> correctly for other macros. Sometimes a function vector is called
> directly by macro (using another macro ;), and sometimes its a real
> function which then calls the function pointer. And the list goes on.

Yeah, well that's bfd for you. :)

> BTW, I didn't realize until recently that aoutx.h can be used outside
> of aout32.c/aout64.c. However I was surprised to see that aout-cris.c
> does include aout32.c directly. I guess that was a deliberate decision?

I honestly can't remember.  Doesn't it match the pattern of
other aout-*.c files?  ...hm, not many around.  Oh wait: I think
something was sufficiently different that I couldn't use
aout32.c, perhaps the reloc format.

> Anyway including aoutx.h in my custom backend avoided ugly changes to
> aoutx.h itself. That was a surprising lesson.

I think it's actually meant to be used that way, directly or
via aout32.h/aout64.h.  At least it says so in a comment.

brgds, H-P

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Wed, 6 May 2020, Gunther Nikl wrote:
> > BTW, I didn't realize until recently that aoutx.h can be used
> > outside of aout32.c/aout64.c. However I was surprised to see that
> > aout-cris.c does include aout32.c directly. I guess that was a
> > deliberate decision?
> 
> I honestly can't remember.  Doesn't it match the pattern of
> other aout-*.c files?  ...hm, not many around.  Oh wait: I think
> something was sufficiently different that I couldn't use
> aout32.c, perhaps the reloc format.

Oh, that much I understood. I meant that aout-cris.c does

  #include "aout32.c"

when I expected

  #include "aoutx.h"

Since I was grepping for the header the result list did not include
aout-cris.c.

A last question if you don't mind: there is a comment in set_sizes in
front of line setting the relocation entry size. Since you had to add
a case of bfd_arch_cris to "NAME (aout, machine_type)" adding such a
case to "NAME (aout, set_arch_mach)" to use the generic set_sizes does
not sound that bad. At least for me the machine_type function is also
about target-specific things.

> > Anyway including aoutx.h in my custom backend avoided ugly changes
> > to aoutx.h itself. That was a surprising lesson.
> 
> I think it's actually meant to be used that way, directly or
> via aout32.h/aout64.h.  At least it says so in a comment.

I am not sure that this was the envisioned usage for aoutx.h. I always
thought an a.out backend had to use the exported functions from aoutXX.o
and with the header aout64.c could simply use the same implementation.
However I missed all the fancy #defines in aoutx.h which only make
sense if aoutx.h is used in other files besides aoutXX.c.

Regards,
Gunther

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Hans-Peter Nilsson]

On Thu, 7 May 2020, Gunther Nikl wrote:
> Hans-Peter Nilsson <hp@bitrange.com> wrote:
> > On Wed, 6 May 2020, Gunther Nikl wrote:
> > > BTW, I didn't realize until recently that aoutx.h can be used
> > > outside of aout32.c/aout64.c. However I was surprised to see that
> > > aout-cris.c does include aout32.c directly. I guess that was a
> > > deliberate decision?
> >
> > I honestly can't remember.  Doesn't it match the pattern of
> > other aout-*.c files?  ...hm, not many around.  Oh wait: I think
> > something was sufficiently different that I couldn't use
> > aout32.c, perhaps the reloc format.
>
> Oh, that much I understood. I meant that aout-cris.c does
>
>   #include "aout32.c"
>
> when I expected
>
>   #include "aoutx.h"

Looks like I misremembered.  I *guess* I thought "inheriting"
from the more specific "class" (aout32.c) was cleaner than
including aoutx.h.  I still do. :)

> Since I was grepping for the header the result list did not include
> aout-cris.c.
>
> A last question if you don't mind: there is a comment in set_sizes in
> front of line setting the relocation entry size. Since you had to add
> a case of bfd_arch_cris to "NAME (aout, machine_type)" adding such a
> case to "NAME (aout, set_arch_mach)" to use the generic set_sizes does
> not sound that bad. At least for me the machine_type function is also
> about target-specific things.

I didn't see a question there.  Were the comments unclear?
I think I understand them.

brgds, H-P

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Hans-Peter Nilsson <hp@bitrange.com>:
>
> On Thu, 7 May 2020, Gunther Nikl wrote:
> >
> > A last question if you don't mind: there is a comment in set_sizes
> > in front of line setting the relocation entry size. Since you had
> > to add a case of bfd_arch_cris to "NAME (aout, machine_type)"
> > adding such a case to "NAME (aout, set_arch_mach)" to use the
> > generic set_sizes does not sound that bad. At least for me the
> > machine_type function is also about target-specific things.
> 
> I didn't see a question there.  Were the comments unclear?
> I think I understand them.

The question is: why is adding the bfd_arch_cris case in "NAME (aout,
machine_type)" ok, but extending the reloc case in "NAME (aout,
set_arch_mach)" is not?

Regards,
Gunther

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Hans-Peter Nilsson]

On Thu, 7 May 2020, Gunther Nikl wrote:

> Hans-Peter Nilsson <hp@bitrange.com>:
> >
> > On Thu, 7 May 2020, Gunther Nikl wrote:
> > >
> > > A last question if you don't mind: there is a comment in set_sizes
> > > in front of line setting the relocation entry size. Since you had
> > > to add a case of bfd_arch_cris to "NAME (aout, machine_type)"
> > > adding such a case to "NAME (aout, set_arch_mach)" to use the
> > > generic set_sizes does not sound that bad. At least for me the
> > > machine_type function is also about target-specific things.
> >
> > I didn't see a question there.  Were the comments unclear?
> > I think I understand them.
>
> The question is: why is adding the bfd_arch_cris case in "NAME (aout,
> machine_type)" ok,

Where is that?  (If you mistype something, I likely cannot
autocorrect.)

> but extending the reloc case in "NAME (aout,
> set_arch_mach)" is not?

(I guess here you mean the site you patched.)

brgds, H-P

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Hans-Peter Nilsson]

On Thu, 7 May 2020, Hans-Peter Nilsson wrote:
> On Thu, 7 May 2020, Gunther Nikl wrote:
>
> > Hans-Peter Nilsson <hp@bitrange.com>:
> > >
> > > On Thu, 7 May 2020, Gunther Nikl wrote:
> > > >
> > > > A last question if you don't mind: there is a comment in set_sizes
> > > > in front of line setting the relocation entry size. Since you had
> > > > to add a case of bfd_arch_cris to "NAME (aout, machine_type)"
> > > > adding such a case to "NAME (aout, set_arch_mach)" to use the
> > > > generic set_sizes does not sound that bad. At least for me the
> > > > machine_type function is also about target-specific things.
> > >
> > > I didn't see a question there.  Were the comments unclear?
> > > I think I understand them.
> >
> > The question is: why is adding the bfd_arch_cris case in "NAME (aout,
> > machine_type)" ok,
>
> Where is that?  (If you mistype something, I likely cannot
> autocorrect.)

Oh, in aoutx.h.  Please excuse my blindness, don't forget it's
about 20-25 years since I browsed the a.out parts.

> > but extending the reloc case in "NAME (aout,
> > set_arch_mach)" is not?

(Also in aoutx.h)

I see.  One is a designated place where you're *expected* to add
a blurb for your machine, the other is a random place not every
port has to edit and where I found a cleaner way than to IMHO
uglify generic code with another target special-case.  Sure, a
hair-thin difference, but still.  About the same reason you
don't #ifdef AOUT_CRIS all over generic code just because you
need something to be different for your port, but instead either
use an existing mechanism or add a mechanism, hook, or ehatever
you call it, and use that.  That said, both places should
perhaps be better replaced by something that was easier on your
eyes.

That answer will probably go for most other "why did you do X
back then" cases.  Also, sometimes the code could have looked
different back then; some hook that should have been used didn't
exist at the time or whatever.

brgds, H-P

Re: [PATCH] bfd: tweak SET_ARCH_MACH of aout-cris.c

[Gunther Nikl]

Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Thu, 7 May 2020, Hans-Peter Nilsson wrote:
> > On Thu, 7 May 2020, Gunther Nikl wrote:
> >
> > > Hans-Peter Nilsson <hp@bitrange.com>:
> > > >
> > > > On Thu, 7 May 2020, Gunther Nikl wrote:
> > > > >
> > > > > A last question if you don't mind: there is a comment in
> > > > > set_sizes in front of line setting the relocation entry size.
> > > > > Since you had to add a case of bfd_arch_cris to "NAME (aout,
> > > > > machine_type)" adding such a case to "NAME (aout,
> > > > > set_arch_mach)" to use the generic set_sizes does not sound
> > > > > that bad. At least for me the machine_type function is also
> > > > > about target-specific things.
> > > >
> > > > I didn't see a question there.  Were the comments unclear?
> > > > I think I understand them.
> > >
> > > The question is: why is adding the bfd_arch_cris case in "NAME
> > > (aout, machine_type)" ok,
> >
> > Where is that?  (If you mistype something, I likely cannot
> > autocorrect.)
> 
> Oh, in aoutx.h.  Please excuse my blindness, don't forget it's
> about 20-25 years since I browsed the a.out parts.

Don't worry. I know very well that this is old code. When reading the
submission mails for the CRIS port you wrote that the port was from the
1992 time frame. Thats pretty old by now :)

> > > but extending the reloc case in "NAME (aout,
> > > set_arch_mach)" is not?
> 
> (Also in aoutx.h)
> 
> I see.  One is a designated place where you're *expected* to add
> a blurb for your machine, the other is a random place not every
> port has to edit and where I found a cleaner way than to IMHO
> uglify generic code with another target special-case.  Sure, a
> hair-thin difference, but still.  About the same reason you
> don't #ifdef AOUT_CRIS all over generic code just because you
> need something to be different for your port, but instead either
> use an existing mechanism or add a mechanism, hook, or ehatever
> you call it, and use that.  That said, both places should
> perhaps be better replaced by something that was easier on your
> eyes.

Thank you for these insights. I agree that target specific settings
should be in a target file. In this case I would probably have chosen
the easy way and modified the generic code since it already had such
target dependent code. What I have learned while studying the sources:
most of the time fixes/improvements are done to generic code which all
users of tht code get for free. A copy in another file is easily
overlooked.

> That answer will probably go for most other "why did you do X
> back then" cases.  Also, sometimes the code could have looked
> different back then; some hook that should have been used didn't
> exist at the time or whatever.

Regards,
Gunther