ARM long branch stubs: cleanup

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

ARM long branch stubs: cleanup

Christophe Lyon
Hi all,

With the attached small patch, I propose to remove an error-prone switch
statement to get the correct stub template and size in arm_size_one_stub().
It is replaced by an (error-prone :-) array defined next to the stub
themselves.

Maybe it would save some maintenance issues.

This is a subjective and cosmetic patch :-)

Christophe.

2009-03-06  Christophe lyon  <[hidden email]>
        bfd/
        * elf32-arm.c (stub_def): New type.
        (stub_definition): New array, containing stub template pointers
        and sizes.
        (arm_size_one_stub): Make use of stub_definitions.

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.180
diff -u -p -r1.180 elf32-arm.c
--- bfd/elf32-arm.c 6 Mar 2009 08:57:57 -0000 1.180
+++ bfd/elf32-arm.c 6 Mar 2009 17:02:57 -0000
@@ -2155,6 +2155,31 @@ enum elf32_arm_stub_type
   arm_stub_long_branch_thumb_only_pic,
 };
 
+typedef struct
+{
+  const insn_sequence* template;
+  int template_size;
+}  stub_def;
+
+#define STUB_DEF(A) {(A), ARRAY_SIZE(A)}
+
+/* Caution: this array has to be kept in sync with the above stub
+   definitions.  */
+static const stub_def stub_definitions[] =
+  {
+    {NULL, 0},
+    STUB_DEF(elf32_arm_stub_long_branch_any_any),
+    STUB_DEF(elf32_arm_stub_long_branch_v4t_arm_thumb),
+    STUB_DEF(elf32_arm_stub_long_branch_thumb_only),
+    STUB_DEF(elf32_arm_stub_long_branch_v4t_thumb_arm),
+    STUB_DEF(elf32_arm_stub_short_branch_v4t_thumb_arm),
+    STUB_DEF(elf32_arm_stub_long_branch_any_arm_pic),
+    STUB_DEF(elf32_arm_stub_long_branch_any_thumb_pic),
+    STUB_DEF(elf32_arm_stub_long_branch_v4t_arm_thumb_pic),
+    STUB_DEF(elf32_arm_stub_long_branch_v4t_thumb_arm_pic),
+    STUB_DEF(elf32_arm_stub_long_branch_thumb_only_pic)
+  };
+
 struct elf32_arm_stub_hash_entry
 {
   /* Base hash table entry structure.  */
@@ -3322,52 +3358,11 @@ arm_size_one_stub (struct bfd_hash_entry
   stub_entry = (struct elf32_arm_stub_hash_entry *) gen_entry;
   htab = (struct elf32_arm_link_hash_table *) in_arg;
 
-  switch (stub_entry->stub_type)
-    {
-    case arm_stub_long_branch_any_any:
-      template =  elf32_arm_stub_long_branch_any_any;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_any);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb:
-      template =  elf32_arm_stub_long_branch_v4t_arm_thumb;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb);
-      break;
-    case arm_stub_long_branch_thumb_only:
-      template =  elf32_arm_stub_long_branch_thumb_only;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_long_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_short_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_short_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_short_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_long_branch_any_arm_pic:
-      template = elf32_arm_stub_long_branch_any_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_arm_pic);
-      break;
-    case arm_stub_long_branch_any_thumb_pic:
-      template = elf32_arm_stub_long_branch_any_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb_pic:
-      template = elf32_arm_stub_long_branch_v4t_arm_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm_pic:
-      template = elf32_arm_stub_long_branch_v4t_thumb_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm_pic);
-      break;
-    case arm_stub_long_branch_thumb_only_pic:
-      template = elf32_arm_stub_long_branch_thumb_only_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only_pic);
-      break;
-    default:
-      BFD_FAIL ();
-      return FALSE;
-    }
+  BFD_ASSERT((stub_entry->stub_type > arm_stub_none)
+     && stub_entry->stub_type < ARRAY_SIZE(stub_definitions));
+
+  template = stub_definitions[stub_entry->stub_type].template;
+  template_size = stub_definitions[stub_entry->stub_type].template_size;
 
   size = 0;
   for (i = 0; i < template_size; i++)
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Paul Brook
On Friday 06 March 2009, Christophe LYON wrote:
> Hi all,
>
> With the attached small patch, I propose to remove an error-prone switch
> statement to get the correct stub template and size in arm_size_one_stub().
> It is replaced by an (error-prone :-) array defined next to the stub
> themselves.
>
> Maybe it would save some maintenance issues.

If you rework the preprocessor macros a bit you can generate both the enum and
the array from the same list.

Something along the lines of
#define DEF_STUBS DEF_STUB(long_branch_any)[...]

#define DEF_STUB(x) arm_stub_##x,
enum{
  DEF_STUBS
}
#undef DEF_STUB

#define DEF_STUB(x) elf32_arm_stub_##x,
static const stub_def stub_definitions[] = {
 DEF_STUBS
}

Paul
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Daniel Jacobowitz-2
On Mon, Mar 09, 2009 at 01:03:57PM +0000, Paul Brook wrote:

> On Friday 06 March 2009, Christophe LYON wrote:
> > Hi all,
> >
> > With the attached small patch, I propose to remove an error-prone switch
> > statement to get the correct stub template and size in arm_size_one_stub().
> > It is replaced by an (error-prone :-) array defined next to the stub
> > themselves.
> >
> > Maybe it would save some maintenance issues.
>
> If you rework the preprocessor macros a bit you can generate both the enum and
> the array from the same list.

But then you get backslashes all over the place - is it an
improvement?

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Christophe Lyon
In reply to this post by Paul Brook
Hi Paul,

> If you rework the preprocessor macros a bit you can generate both the enum and
> the array from the same list.
>

Thanks for your suggestion. Here is a new proposal.

Christophe.

2009-03-06  Christophe lyon  <[hidden email]>
        bfd/
        * elf32-arm.c (stub_def): New type.
        (stub_definitions): New array, containing stub template pointers
        and sizes.
        (arm_size_one_stub): Make use of stub_definitions.

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.180
diff -u -p -r1.180 elf32-arm.c
--- bfd/elf32-arm.c 6 Mar 2009 08:57:57 -0000 1.180
+++ bfd/elf32-arm.c 9 Mar 2009 13:24:08 -0000
@@ -2140,19 +2140,36 @@ static const insn_sequence elf32_arm_stu
    string.  */
 #define STUB_SUFFIX ".stub"
 
-enum elf32_arm_stub_type
-{
+/* One entry per long/short branch stub defined above.  */
+#define DEF_STUBS \
+  DEF_STUB(long_branch_any_any) \
+  DEF_STUB(long_branch_v4t_arm_thumb) \
+  DEF_STUB(long_branch_thumb_only) \
+  DEF_STUB(long_branch_v4t_thumb_arm) \
+  DEF_STUB(short_branch_v4t_thumb_arm) \
+  DEF_STUB(long_branch_any_arm_pic) \
+  DEF_STUB(long_branch_any_thumb_pic) \
+  DEF_STUB(long_branch_v4t_arm_thumb_pic) \
+  DEF_STUB(long_branch_v4t_thumb_arm_pic) \
+  DEF_STUB(long_branch_thumb_only_pic)
+
+#define DEF_STUB(x) arm_stub_##x,
+enum elf32_arm_stub_type {
   arm_stub_none,
-  arm_stub_long_branch_any_any,
-  arm_stub_long_branch_v4t_arm_thumb,
-  arm_stub_long_branch_thumb_only,
-  arm_stub_long_branch_v4t_thumb_arm,
-  arm_stub_short_branch_v4t_thumb_arm,
-  arm_stub_long_branch_any_arm_pic,
-  arm_stub_long_branch_any_thumb_pic,
-  arm_stub_long_branch_v4t_arm_thumb_pic,
-  arm_stub_long_branch_v4t_thumb_arm_pic,
-  arm_stub_long_branch_thumb_only_pic,
+  DEF_STUBS
+};
+#undef DEF_STUB
+
+typedef struct
+{
+  const insn_sequence* template;
+  int template_size;
+} stub_def;
+
+#define DEF_STUB(x) {elf32_arm_stub_##x, ARRAY_SIZE(elf32_arm_stub_##x)},
+static const stub_def stub_definitions[] = {
+  {NULL, 0},
+  DEF_STUBS
 };
 
 struct elf32_arm_stub_hash_entry
@@ -3322,52 +3350,11 @@ arm_size_one_stub (struct bfd_hash_entry
   stub_entry = (struct elf32_arm_stub_hash_entry *) gen_entry;
   htab = (struct elf32_arm_link_hash_table *) in_arg;
 
-  switch (stub_entry->stub_type)
-    {
-    case arm_stub_long_branch_any_any:
-      template =  elf32_arm_stub_long_branch_any_any;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_any);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb:
-      template =  elf32_arm_stub_long_branch_v4t_arm_thumb;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb);
-      break;
-    case arm_stub_long_branch_thumb_only:
-      template =  elf32_arm_stub_long_branch_thumb_only;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_long_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_short_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_short_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_short_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_long_branch_any_arm_pic:
-      template = elf32_arm_stub_long_branch_any_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_arm_pic);
-      break;
-    case arm_stub_long_branch_any_thumb_pic:
-      template = elf32_arm_stub_long_branch_any_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb_pic:
-      template = elf32_arm_stub_long_branch_v4t_arm_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm_pic:
-      template = elf32_arm_stub_long_branch_v4t_thumb_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm_pic);
-      break;
-    case arm_stub_long_branch_thumb_only_pic:
-      template = elf32_arm_stub_long_branch_thumb_only_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only_pic);
-      break;
-    default:
-      BFD_FAIL ();
-      return FALSE;
-    }
+  BFD_ASSERT((stub_entry->stub_type > arm_stub_none)
+     && stub_entry->stub_type < ARRAY_SIZE(stub_definitions));
+
+  template = stub_definitions[stub_entry->stub_type].template;
+  template_size = stub_definitions[stub_entry->stub_type].template_size;
 
   size = 0;
   for (i = 0; i < template_size; i++)
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Paul Brook
In reply to this post by Daniel Jacobowitz-2
> > > With the attached small patch, I propose to remove an error-prone
> > > switch statement to get the correct stub template and size in
> > > arm_size_one_stub(). It is replaced by an (error-prone :-) array
> > > defined next to the stub themselves.
> > >
> > > Maybe it would save some maintenance issues.
> >
> > If you rework the preprocessor macros a bit you can generate both the
> > enum and the array from the same list.
>
> But then you get backslashes all over the place - is it an
> improvement?

Hmm, maybe? It doesn't look as pretty, but means you don't have to maintain
duplicate lists.

Paul
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Christophe Lyon
In reply to this post by Christophe Lyon
Hi all,

On 09.03.2009 14:28, Christophe LYON wrote:

> Hi Paul,
>
>> If you rework the preprocessor macros a bit you can generate both the
>> enum and the array from the same list.
>>
>
> Thanks for your suggestion. Here is a new proposal.
>
> Christophe.
>


I am not sure we reached a conclusion here?
So, what's the decision?
1- change nothing
2- my 1st proposal
3- my 2nd proposal taking into account Paul's comments, not as pretty
but probably easier to maintain

Christophe.
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Christophe Lyon
Hi all,

In case we come up to an agreement, here is an updated proposal.

It would probably be possible to convert other stubs (for various PLT
variants), but I am afraid it would add some overhead, so it's probably
not worth it.

Christophe.



2009-04-22  Christophe lyon  <[hidden email]>
        bfd/
        * elf32-arm.c (DEF_STUBS): New helper define.
        (DEF_STUB): Likewise.
        (stub_def): New type.
        (stub_definitions): New array, containing stub template pointers
        and sizes.
        (arm_size_one_stub): Make use of stub_definitions.

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.189
diff -u -p -r1.189 elf32-arm.c
--- bfd/elf32-arm.c 22 Apr 2009 14:01:30 -0000 1.189
+++ bfd/elf32-arm.c 22 Apr 2009 15:37:28 -0000
@@ -2166,21 +2166,38 @@ static const insn_sequence elf32_arm_stu
    string.  */
 #define STUB_SUFFIX ".stub"
 
-enum elf32_arm_stub_type
-{
+/* One entry per long/short branch stub defined above.  */
+#define DEF_STUBS \
+  DEF_STUB(long_branch_any_any) \
+  DEF_STUB(long_branch_v4t_arm_thumb) \
+  DEF_STUB(long_branch_thumb_only) \
+  DEF_STUB(long_branch_v4t_thumb_thumb) \
+  DEF_STUB(long_branch_v4t_thumb_arm) \
+  DEF_STUB(short_branch_v4t_thumb_arm) \
+  DEF_STUB(long_branch_any_arm_pic) \
+  DEF_STUB(long_branch_any_thumb_pic) \
+  DEF_STUB(long_branch_v4t_thumb_thumb_pic) \
+  DEF_STUB(long_branch_v4t_arm_thumb_pic) \
+  DEF_STUB(long_branch_v4t_thumb_arm_pic) \
+  DEF_STUB(long_branch_thumb_only_pic)
+
+#define DEF_STUB(x) arm_stub_##x,
+enum elf32_arm_stub_type {
   arm_stub_none,
-  arm_stub_long_branch_any_any,
-  arm_stub_long_branch_v4t_arm_thumb,
-  arm_stub_long_branch_thumb_only,
-  arm_stub_long_branch_v4t_thumb_thumb,
-  arm_stub_long_branch_v4t_thumb_arm,
-  arm_stub_short_branch_v4t_thumb_arm,
-  arm_stub_long_branch_any_arm_pic,
-  arm_stub_long_branch_any_thumb_pic,
-  arm_stub_long_branch_v4t_arm_thumb_pic,
-  arm_stub_long_branch_v4t_thumb_arm_pic,
-  arm_stub_long_branch_thumb_only_pic,
-  arm_stub_long_branch_v4t_thumb_thumb_pic,
+  DEF_STUBS
+};
+#undef DEF_STUB
+
+typedef struct
+{
+  const insn_sequence* template;
+  int template_size;
+} stub_def;
+
+#define DEF_STUB(x) {elf32_arm_stub_##x, ARRAY_SIZE(elf32_arm_stub_##x)},
+static const stub_def stub_definitions[] = {
+  {NULL, 0},
+  DEF_STUBS
 };
 
 struct elf32_arm_stub_hash_entry
@@ -3361,60 +3378,11 @@ arm_size_one_stub (struct bfd_hash_entry
   stub_entry = (struct elf32_arm_stub_hash_entry *) gen_entry;
   htab = (struct elf32_arm_link_hash_table *) in_arg;
 
-  switch (stub_entry->stub_type)
-    {
-    case arm_stub_long_branch_any_any:
-      template =  elf32_arm_stub_long_branch_any_any;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_any);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb:
-      template =  elf32_arm_stub_long_branch_v4t_arm_thumb;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb);
-      break;
-    case arm_stub_long_branch_thumb_only:
-      template =  elf32_arm_stub_long_branch_thumb_only;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only);
-      break;
-    case arm_stub_long_branch_v4t_thumb_thumb:
-      template =  elf32_arm_stub_long_branch_v4t_thumb_thumb;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_thumb);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_long_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_short_branch_v4t_thumb_arm:
-      template =  elf32_arm_stub_short_branch_v4t_thumb_arm;
-      template_size = ARRAY_SIZE (elf32_arm_stub_short_branch_v4t_thumb_arm);
-      break;
-    case arm_stub_long_branch_any_arm_pic:
-      template = elf32_arm_stub_long_branch_any_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_arm_pic);
-      break;
-    case arm_stub_long_branch_any_thumb_pic:
-      template = elf32_arm_stub_long_branch_any_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_any_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_arm_thumb_pic:
-      template = elf32_arm_stub_long_branch_v4t_arm_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_arm_thumb_pic);
-      break;
-    case arm_stub_long_branch_v4t_thumb_arm_pic:
-      template = elf32_arm_stub_long_branch_v4t_thumb_arm_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_arm_pic);
-      break;
-    case arm_stub_long_branch_thumb_only_pic:
-      template = elf32_arm_stub_long_branch_thumb_only_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_thumb_only_pic);
-      break;
-    case arm_stub_long_branch_v4t_thumb_thumb_pic:
-      template = elf32_arm_stub_long_branch_v4t_thumb_thumb_pic;
-      template_size = ARRAY_SIZE (elf32_arm_stub_long_branch_v4t_thumb_thumb_pic);
-      break;
-    default:
-      BFD_FAIL ();
-      return FALSE;
-    }
+  BFD_ASSERT((stub_entry->stub_type > arm_stub_none)
+     && stub_entry->stub_type < ARRAY_SIZE(stub_definitions));
+
+  template = stub_definitions[stub_entry->stub_type].template;
+  template_size = stub_definitions[stub_entry->stub_type].template_size;
 
   size = 0;
   for (i = 0; i < template_size; i++)
Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Nick Clifton
Hi Christophe,

> In case we come up to an agreement, here is an updated proposal.

This patch is approved.

Cheers
   Nick


Reply | Threaded
Open this post in threaded view
|

Re: ARM long branch stubs: cleanup

Christophe Lyon
On 05.05.2009 12:29, Nick Clifton wrote:
> Hi Christophe,
>
>> In case we come up to an agreement, here is an updated proposal.
>
> This patch is approved.
>

Thanks for the review. I have just committed it.

Christophe.