RFA: probable rs6000-aix-tdep.c bug found by clang

RFA: probable rs6000-aix-tdep.c bug found by clang

[Tom Tromey]

Here is another patch for a bug found by clang.

rs6000-aix-tdep.c:rs6000_aix_osabi_sniffer has an extraneous ";" that
makes the initial "if" irrelevant.

Based on indentation and logic I think that the fix is to remove the ";".
However, I have no decent way to test this and would appreciate someone
else looking at it.

Tom

2012-10-17  Tom Tromey  <tromey@redhat.com>

	* rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Remove extraneous
	semicolon.

diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 59cfa73..749c109 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -723,7 +723,7 @@ static enum gdb_osabi
 rs6000_aix_osabi_sniffer (bfd *abfd)
 {
   
-  if (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour);
+  if (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour)
     return GDB_OSABI_AIX;
 
   return GDB_OSABI_UNKNOWN;

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Joel Brobecker]

> Based on indentation and logic I think that the fix is to remove the ";".
> However, I have no decent way to test this and would appreciate someone
> else looking at it.
[...]
> 2012-10-17  Tom Tromey  <tromey@redhat.com>
> 
> 	* rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Remove extraneous
> 	semicolon.

Thanks Tom, for shaming my Evil twin :-). He's pretending to be
spelunking at the moment - aka hiding in a cave, while his Good
twin is writing a reply...

The patch looks completely correct. In fact, AdaCore's version of GDB
defines the function as follow:

  if (bfd_get_flavour (abfd) != bfd_target_xcoff_flavour)
    return GDB_OSABI_UNKNOWN;

  /* The only noticeable difference between Lynx178 XCOFF files and
     AIX XCOFF files comes from the fact that there are no shared
     libraries on Lynx178. On AIX, we are betting that an executable
     linked with no shared library will never exist.  */
  if (xcoff_get_n_import_files (abfd) <= 0)
    return GDB_OSABI_UNKNOWN;

  return GDB_OSABI_AIX;

We've had that change for a few months, now, so that should be
enough evidence that the change is not going to create problems.

I researched a little bit how this error managed to not break any
other platform.  But I think we just got lucky, because no other
powerpc target outside of AIX uses the xcoff flavor.

In fact, going one step further, do we need the check at all?
This is how the sniffer is registered:

  gdbarch_register_osabi_sniffer (bfd_arch_rs6000,
                                  bfd_target_xcoff_flavour,
                                  rs6000_aix_osabi_sniffer);
  gdbarch_register_osabi_sniffer (bfd_arch_powerpc,
                                  bfd_target_xcoff_flavour,
                                  rs6000_aix_osabi_sniffer);

So, isn't rs6000_aix_osabi_sniffer going to be called if, and only
if, the bfd has a bfd_target_xcoff_flavour, thus making the check
superfluous?


> 
> diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
> index 59cfa73..749c109 100644
> --- a/gdb/rs6000-aix-tdep.c
> +++ b/gdb/rs6000-aix-tdep.c
> @@ -723,7 +723,7 @@ static enum gdb_osabi
>  rs6000_aix_osabi_sniffer (bfd *abfd)
>  {
>    
> -  if (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour);
> +  if (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour)
>      return GDB_OSABI_AIX;
>  
>    return GDB_OSABI_UNKNOWN;

-- 
Joel

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Pedro Alves]

On 10/17/2012 10:24 PM, Joel Brobecker wrote:
> 
>> Based on indentation and logic I think that the fix is to remove the ";".
>> However, I have no decent way to test this and would appreciate someone
>> else looking at it.
> [...]
>> 2012-10-17  Tom Tromey  <tromey@redhat.com>
>>
>> 	* rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Remove extraneous
>> 	semicolon.

I suspect this is the root cause of PR8966 (AIX 5.1 single_step configuration
is broken).  http://sourceware.org/bugzilla/show_bug.cgi?id=8966

We're always returning the _UNKNOWN osabi, so this is unreachable:

static void
rs6000_aix_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
{
  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

  /* RS6000/AIX does not support PT_STEP.  Has to be simulated.  */
  set_gdbarch_software_single_step (gdbarch, rs6000_software_single_step);
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> In fact, going one step further, do we need the check at all?
> This is how the sniffer is registered:
> 
>   gdbarch_register_osabi_sniffer (bfd_arch_rs6000,
>                                   bfd_target_xcoff_flavour,
>                                   rs6000_aix_osabi_sniffer);
>   gdbarch_register_osabi_sniffer (bfd_arch_powerpc,
>                                   bfd_target_xcoff_flavour,
>                                   rs6000_aix_osabi_sniffer);
> 
> So, isn't rs6000_aix_osabi_sniffer going to be called if, and only
> if, the bfd has a bfd_target_xcoff_flavour, thus making the check
> superfluous?

Sure looks like it.

-- 
Pedro Alves

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Joel Brobecker]

> >> 	* rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Remove extraneous
> >> 	semicolon.
> 
> I suspect this is the root cause of PR8966 (AIX 5.1 single_step configuration
> is broken).  http://sourceware.org/bugzilla/show_bug.cgi?id=8966
> 
> We're always returning the _UNKNOWN osabi, so this is unreachable:

On the contrary, I think we always returned the AIX osabi.

> > So, isn't rs6000_aix_osabi_sniffer going to be called if, and only
> > if, the bfd has a bfd_target_xcoff_flavour, thus making the check
> > superfluous?
> 
> Sure looks like it.

OK - I will take care of removing the condition and simplifying
the function.

-- 
Joel

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Pedro Alves]

On 10/18/2012 04:33 PM, Joel Brobecker wrote:
>>>> 	* rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Remove extraneous
>>>> 	semicolon.
>>
>> I suspect this is the root cause of PR8966 (AIX 5.1 single_step configuration
>> is broken).  http://sourceware.org/bugzilla/show_bug.cgi?id=8966
>>
>> We're always returning the _UNKNOWN osabi, so this is unreachable:
> 
> On the contrary, I think we always returned the AIX osabi.

Duh.  You're right.

I now notice the PR predates the patch that added this (a8cfed2a), and it seems the
patch was meant to fix exactly that single-step issue in the PR.  So I guess we
could close it.

Searching bugzilla for aix finds a handful of PRs, and most looks quite old.  I'd be
nice to give them a little triage love.  :-)

>>> So, isn't rs6000_aix_osabi_sniffer going to be called if, and only
>>> if, the bfd has a bfd_target_xcoff_flavour, thus making the check
>>> superfluous?
>>
>> Sure looks like it.
> 
> OK - I will take care of removing the condition and simplifying
> the function.

-- 
Pedro Alves

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Joel Brobecker]

> I now notice the PR predates the patch that added this (a8cfed2a), and
> it seems the patch was meant to fix exactly that single-step issue in
> the PR.  So I guess we could close it.

That was the first thing I started looking for when I eventually
checked the PR :-). I closed the ticket.

> Searching bugzilla for aix finds a handful of PRs, and most looks
> quite old.  I'd be nice to give them a little triage love.  :-)

I can add that to my list...

-- 
Joel

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Pedro Alves]

On 10/18/2012 04:57 PM, Joel Brobecker wrote:
>> I now notice the PR predates the patch that added this (a8cfed2a), and
>> it seems the patch was meant to fix exactly that single-step issue in
>> the PR.  So I guess we could close it.
> 
> That was the first thing I started looking for when I eventually
> checked the PR :-). I closed the ticket.
> 
>> Searching bugzilla for aix finds a handful of PRs, and most looks
>> quite old.  I'd be nice to give them a little triage love.  :-)
> 
> I can add that to my list...

Thanks.  :-)

-- 
Pedro Alves

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

> OK - I will take care of removing the condition and simplifying
> the function.

Here is what I propose we check in.  I can't test it at the moment,
because we've suffered a massive power outage and we're still working
on getting every machine back up. I'm fairly confident, though.

Rather than remove the check, I added an assertion. That's because
eventually I want to add an extra check, for Lynx178. On the GDB side,
the Lynx178 port is roughly a bareboard powerpc configuration. But
it is using XCOFF, and I wanted to make sure that we'd never match
a Lynx178 binary with the AIX ABI. To do that, I use a BFD/XCOFF
function, so I thought adding the explict assertion would help.

While at it, I added a function description, that explicitly mentions
the pre-condition/assertion.

If there are no objections, I will commit the attached patch within
the next few days, after being able to test on AIX again.

Thanks,
-- 
Joel

[-- Attachment #2: 0001-ppc-aix-osabi-sniffer-Turn-test-of-bfd-flavour-into-.patch --]
[-- Type: text/x-diff, Size: 1755 bytes --]

From a2aff2ac36f9573ede236e899994054e6194ba65 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 18 Oct 2012 11:31:28 -0700
Subject: [PATCH] ppc-aix osabi sniffer: Turn test of bfd flavour into
 assertion

Due to the way this function is registers, we know that given bfd's
flavour should always be bfd_target_xcoff_flavour, thus making
the former test always true, which means that this function should
always return GDB_OSABI_AIX, and never return GDB_OSABI_UNKNOWN.

This patch also fixes a typo detected by Tom Tromey that caused
the test itself to be completely ineffective.

gdb/ChangeLog (by Tom Tromey and Joel Brobecker):

        * rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Replace
        inneffective if condition by gdb assertion.  Add function
        description comment.
---
 gdb/rs6000-aix-tdep.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 59cfa73..06b43de 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -719,14 +719,19 @@ rs6000_software_single_step (struct frame_info *frame)
   return 1;
 }
 
+/* Implement an osabi sniffer for RS6000/AIX.
+
+   This function assumes that ABFD's flavour is XCOFF.  In other words,
+   it should be registered as a sniffer for bfd_target_xcoff_flavour
+   objfiles only.  A failed assertion will be raised if this condition
+   is not met.  */
+
 static enum gdb_osabi
 rs6000_aix_osabi_sniffer (bfd *abfd)
 {
-  
-  if (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour);
-    return GDB_OSABI_AIX;
+  gdb_assert (bfd_get_flavour (abfd) == bfd_target_xcoff_flavour);
 
-  return GDB_OSABI_UNKNOWN;
+  return GDB_OSABI_AIX;
 }
 
 static void
-- 
1.7.9.5

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> If there are no objections, I will commit the attached patch within
Joel> the next few days, after being able to test on AIX again.

Based on my reading of gdbarch_lookup_osabi, this seems reasonable to me.
Thanks, Joel.

Tom

Re: RFA: probable rs6000-aix-tdep.c bug found by clang

[Joel Brobecker]

> gdb/ChangeLog (by Tom Tromey and Joel Brobecker):
> 
>         * rs6000-aix-tdep.c (rs6000_aix_osabi_sniffer): Replace
>         inneffective if condition by gdb assertion.  Add function
>         description comment.

I confirm that testing revealed no issue, so checked in.
As usual on AIX, I've only been able to run the AdaCore testsuite,
which should be good enough for this particular change.

-- 
Joel