diff mbox

Remove some obfuscation from ${arch}_skip_prologue functions

Message ID yjt2y4u1pokj.fsf@ruffy.mtv.corp.google.com
State New, archived
Headers

Commit Message

Doug Evans Sept. 2, 2014, 11:53 p.m. UTC 
  Hi.

This patch is a follow up to an observation made here:
https://sourceware.org/ml/gdb-patches/2014-08/msg00539.html

Basically, I think there's either or both of some
bitrot/misunderstanding in the prologue parsers.

1) There's no need to call find_pc_partial_function before
calling skip_prologue_using_sal: The first thing skip_prologue_using_sal
does is call find_pc_partial_function!
[There is another reason for ${arch}_skip_prologue routines to call
find_pc_partial_function: to find the end address of the function
to set an upper bound on how many insns to parse.  Some routines
do this, e.g., rs6000_skip_prologue, and I think some other routines
*could* do this (grep for FIXME) in the patch.  However, I have not done
this here because I wanted to restrict the patch to unobfuscation.
This patch is intended to be a functional no-op - ASSUMING my observation is
correct.]

2) Several ${arch}_skip_prologue routines call skip_prologue_using_sal
a second time if the first one fails in order to find an upper bound on
insns to scan.  I don't see how this can work:
If either find_pc_partial_function or the first call to
skip_prologue_using_sal fails, then the second call to
skip_prologue_using_sal is going to fail too.

I should also make clear that I'm totally willing to believe the
misunderstanding is mine!  I've gone through this code several times
trying to find a situation I'm missing and I can't.  Plus I've regression
tested this patch on amd64, i386, ppc64, aarch64 (and build with
--enable-targets=all).  So I think the observation is valid,
but of course the testsuite may not cover the case that
is the reason why the code is the way it is currently.
At the very least, I'd like to document Why Things Are The Way They Are
if there is indeed a reason for it.

There may still be bugs in the prologue parsers, but I would like to
unobfuscate them first.  Again, this patch is believed to be a
functional no-op, it just simplifies/unobfuscates the code.

Can I enlist any of the other arch maintainers to regression test this
there: lm32, mips, arm, nios2, tic6x, tilegx, avr, moxie, sh?
Some have simulators, so I may try to get a full toolchain going
for a few of them, though having tested this on four arches and having
all of the changes being mechanical, I'm ok with not checking all of them.

Arch maintainers I could find from the MAINTAINERS file:
mips: macro@codesourcery.com
moxie: green@moxielogic.com
nios2: yao@codesourcery.com
tic6x:yao@codesourcery.com


2014-09-02  Doug Evans  <dje@google.com>

	* symtab.c (skip_prologue_using_sal): Return zero if
	find_pc_partial_function fails.
	* aarch64-tdep.c (aarch64_skip_prologue): Remove redundant call
	to find_pc_partial_function.  Remove useless second call to
	skip_prologue_using_sal.
	* lm32-tdep.c (lm32_analyze_prologue): Ditto.
	* mips-tdep.c (mips_skip_prologue): Ditto.
	* amd64-tdep.c (amd64_skip_prologue): Remove redundant call to
	find_pc_partial_function.
	* amd64-windows-tdep.c (amd64_windows_skip_prologue): Ditto.
	* arm-tdep.c (arm_skip_prologue): Ditto.
	* i386-tdep.c (i386_skip_prologue): Ditto.
	* nios2-tdep.c (nios2_skip_prologue): Ditto.
	* tic6x-tdep.c (tic6x_analyze_prologue): Ditto.
	* tilegx-tdep.c (tilegx_analyze_prologue): Ditto.
	* avr-tdep.c (avr_skip_prologue): Clean up, call
	skip_prologue_using_sal before find_pc_partial_function.
	* moxie-tdep.c (moxie_analyze_prologue): Ditto.
	* rs6000-tdep.c (rs6000_skip_prologue): Ditto.
	* sh-tdep.c (sh_skip_prologue): Ditto.  Remove useless second call to
	skip_prologue_using_sal.

Comments

Doug Evans Sept. 3, 2014, 5:19 p.m. UTC | #1 
On Tue, Sep 2, 2014 at 4:53 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> This patch is a follow up to an observation made here:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00539.html
>
> [...]
>
> At the very least, I'd like to document Why Things Are The Way They Are
> if there is indeed a reason for it.
>
> [...]

btw, there is still one aspect here that I don't understand the WHY of:

aarch64-tdep.c and others do this (grep for <<<<):

  /* See if we can determine the end of the prologue via the symbol
     table.  If so, then return either PC, or the PC after the
     prologue, whichever is greater.  */
  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
        = skip_prologue_using_sal (gdbarch, func_addr);

      if (post_prologue_pc != 0)
        return max (pc, post_prologue_pc); //<<<<
    }

Why?  Why max (pc, post_prologue_pc) ?

Is it because ${arch}_skip_prologue can be called
with a pc value greater than the start address of the function?
If so, when?
Are people just being conservative because it's not clear
what values of pc may be passed to ${arch}_skip_prologue?
[If so we've got unnecessarily complex code due to unclear APIs ... bleah.]

N.B. There are only two arches that I could find where
(I think) the pc passed to ${arch}_skip_prologue could
be different than the function's entry point:
1) vax, which sets deprecated_function_start_offset
2) ppc-linux (elf v2), which sets skip_entrypoint

If I'm missing something ... great ... let's get it documented.
[And if it is documented, let's make it easier to find. :-)
The prologue stuff I can find from the Internals Wiki doesn't
discuss this, and I wasn't about to read the entire wiki
looking for it.]
Yao Qi Sept. 4, 2014, 8:54 a.m. UTC | #2 
Doug Evans <dje@google.com> writes:

> 1) There's no need to call find_pc_partial_function before
> calling skip_prologue_using_sal: The first thing skip_prologue_using_sal
> does is call find_pc_partial_function!

Nowadays we have:

  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
	= skip_prologue_using_sal (gdbarch, func_addr);

      if (post_prologue_pc != 0)
	return max (pc, post_prologue_pc);
    }

so your statement is valid if PC equals to FUNC_ADDR.  I don't have a
case that PC and FUNC_ADDR are different, but I'd like to add an assert
to check this, in each target's implementation of skip_prologue hook, or
in the callers of gdbarch_skip_prologue, something like:

  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
    gdb_assert (pc == func_addr);

Note that this assert is triggered on arm in
gdb.cp/re-set-overloaded.exp, that is PC is [1] but FUNC_ADDR is [2].

(gdb) disassemble _ZN1CC1Ei
Dump of assembler code for function _ZN1CC1Ev:
   0x0000090c <+0>:     ldr     r12, [pc, #4]   ; 0x918 <_ZN1CC1Ev+12>   <- [2]
   0x00000910 <+4>:     add     r12, r12, pc
   0x00000914 <+8>:     bx      r12
   0x00000918 <+12>:                    ; <UNDEFINED> instruction: 0xffffffc5
   0x0000091c <+0>:     ldr     r12, [pc, #4]   ; 0x928 <_ZN1CC1Ei+12>   <- [1]
   0x00000920 <+4>:     add     r12, r12, pc

AFAICS, PC is still the function address but find_pc_partial_function
computes the FUNC_ADDR incorrectly and it is nothing wrong about your
patch.

> nios2: yao@codesourcery.com

I tested your patch on nios2-linux, and no regression is found.

> tic6x:yao@codesourcery.com

My c6x board is dead in data center, so I can't test this patch for it.
Doug Evans Sept. 4, 2014, 5:42 p.m. UTC | #3 
Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > > 1) There's no need to call find_pc_partial_function before
 > > calling skip_prologue_using_sal: The first thing skip_prologue_using_sal
 > > does is call find_pc_partial_function!
 > 
 > Nowadays we have:
 > 
 >   if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
 >     {
 >       CORE_ADDR post_prologue_pc
 > 	= skip_prologue_using_sal (gdbarch, func_addr);
 > 
 >       if (post_prologue_pc != 0)
 > 	return max (pc, post_prologue_pc);
 >     }
 > 
 > so your statement is valid if PC equals to FUNC_ADDR.

There are other reasons to worry about cases when PC != FUNC_ADDR
(which is why I'm still trying to find examples of when that is true),
but I don't see how this is one of them.
Remember, the first thing skip_prologue_using_sal does is also
call find_pc_partial_function.

So what we have is:

${arch}_skip_prologue calls find_pc_partial_function (PC)
and gets back a start address, called FUNC_ADDR here.

Then,
skip_prologue_using_sal calls find_pc_partial_function (FUNC_ADDR)
and gets back a start address, called START_PC.

Then,
skip_prologue uses START_PC for the rest of the function.

So, under what circumstances is (figuratively speaking):
(find_pc_partial_function (pc)
  != find_pc_partial_function (find_pc_partial_function (pc)))
And if there is such a case, I think we've got another problem ...

btw, note that arm_skip_prologue dropped the max (pc, post_prologue_pc)
check.  It may have been accidental. I found the relevant commit and
emails, it's not clear yet why the check was dropped.

 > I don't have a
 > case that PC and FUNC_ADDR are different, but I'd like to add an assert
 > to check this, in each target's implementation of skip_prologue hook, or
 > in the callers of gdbarch_skip_prologue, something like:
 > 
 >   if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
 >     gdb_assert (pc == func_addr);

I have found two cases where, I think, ${arch}_skip_prologue can
be called with pc != func_addr: vax and ppc.
I'd be happy with simplifying the target API so that
we could have such an assert, though I'd rather not put it
in ${arch}_skip_prologue.  I currently have the problem that
the treatment of gcc vs clang is not as consistent as it could be
across all ${arch}_skip_prologue functions, so I'm on a path of keeping
as much code that should be common out of target-specific routines:
cleaning it up later is not always fun or easy.

 > Note that this assert is triggered on arm in
 > gdb.cp/re-set-overloaded.exp, that is PC is [1] but FUNC_ADDR is [2].
 > 
 > (gdb) disassemble _ZN1CC1Ei
 > Dump of assembler code for function _ZN1CC1Ev:
 >    0x0000090c <+0>:     ldr     r12, [pc, #4]   ; 0x918 <_ZN1CC1Ev+12>   <- [2]
 >    0x00000910 <+4>:     add     r12, r12, pc
 >    0x00000914 <+8>:     bx      r12
 >    0x00000918 <+12>:                    ; <UNDEFINED> instruction: 0xffffffc5
 >    0x0000091c <+0>:     ldr     r12, [pc, #4]   ; 0x928 <_ZN1CC1Ei+12>   <- [1]
 >    0x00000920 <+4>:     add     r12, r12, pc
 > 
 > AFAICS, PC is still the function address but find_pc_partial_function
 > computes the FUNC_ADDR incorrectly and it is nothing wrong about your
 > patch.

Thanks, this is good data.

I did a similar experiment on amd64-linux after writing
https://sourceware.org/ml/gdb-patches/2014-08/msg00539.html
and got no hits.

I'd be curious to see how arm_skip_prologue handles this.
What flavor of arm and what version of gcc?
I can't recreate that example with arm-linux-gnueabi-g++-4.7.
Also, can you send me gdb.log plus re-set-overloaded{,.so} ?
[don't cc the list :-)]

 > 
 > > nios2: yao@codesourcery.com
 > 
 > I tested your patch on nios2-linux, and no regression is found.
 > 
 > > tic6x:yao@codesourcery.com
 > 
 > My c6x board is dead in data center, so I can't test this patch for it.

Thanks!
Maciej W. Rozycki Sept. 4, 2014, 5:51 p.m. UTC | #4 
On Wed, 3 Sep 2014, Doug Evans wrote:

> Can I enlist any of the other arch maintainers to regression test this
> there: lm32, mips, arm, nios2, tic6x, tilegx, avr, moxie, sh?

 Thanks for working on these clean-ups.

 I'll try to push this through testing; what makes me curious about is 
whether there's a hidden trap here for compressed ISAs (i.e. MIPS16 and 
microMIPS code) where the value of the PC is not the same as the memory 
address of the instruction being accessed (the LSB or the ISA bit in the 
PC is forcefully set to 1).  Another matter may be various MIPS16 call 
thunks.  I have a vague recollection of my previous encounters with this 
code, but it was so many years ago I may have troubles bringing back what 
it really was, unless I am able to track down some patches.

 One issue here is not every possible execution path is covered in 
regression testing, some of the issues in places like this only happen in 
situations that are difficult to transform into a proper GDB test case 
(and e.g. our MIPS16 thunk tests remain non-functional with stock GCC 
because of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53276).

  Maciej
diff mbox

Patch

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 1898f6b..3f902b8 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -841,33 +841,22 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 static CORE_ADDR
 aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
-  unsigned long inst;
-  CORE_ADDR skip_pc;
-  CORE_ADDR func_addr, limit_pc;
-  struct symtab_and_line sal;
+  CORE_ADDR limit_pc;
+  CORE_ADDR post_prologue_pc;
 
   /* See if we can determine the end of the prologue via the symbol
      table.  If so, then return either PC, or the PC after the
      prologue, whichever is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
 
-  /* Find an upper limit on the function prologue using the debug
-     information.  If the debug information could not be used to
-     provide that bound, then use an arbitrary large number as the
-     upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    limit_pc = pc + 128;	/* Magic.  */
+  /* Use an arbitrary large number as the upper bound.  */
+  limit_pc = pc + 128;	/* Magic.  */
+  /* FIXME: could do what rs6000-tdep.c does here */
 
   /* Try disassembling prologue.  */
   return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL);
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index abd9c48..84e2c0e 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2359,21 +2359,19 @@  amd64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
   struct amd64_frame_cache cache;
   CORE_ADDR pc;
-  CORE_ADDR func_addr;
+  CORE_ADDR post_prologue_pc;
 
-  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (post_prologue_pc != 0)
     {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      struct symtab *s = find_pc_symtab (func_addr);
+      struct symtab *s = find_pc_symtab (start_pc);
 
       /* Clang always emits a line note before the prologue and another
 	 one after.  We trust clang to emit usable line notes.  */
-      if (post_prologue_pc
-	  && (s != NULL
-	      && s->producer != NULL
-	      && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
-        return max (start_pc, post_prologue_pc);
+      if (s != NULL
+	  && s->producer != NULL
+	  && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0)
+	return max (start_pc, post_prologue_pc);
     }
 
   amd64_init_frame_cache (&cache);
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index f90bd5b..b800666 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1095,9 +1095,9 @@  static const struct frame_unwind amd64_windows_frame_unwind =
 static CORE_ADDR
 amd64_windows_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
-  CORE_ADDR func_addr;
   CORE_ADDR unwind_info = 0;
   CORE_ADDR image_base, start_rva, end_rva;
+  CORE_ADDR post_prologue_pc;
   struct external_pex64_unwind_info ex_ui;
 
   /* Use prologue size from unwind info.  */
@@ -1118,14 +1118,9 @@  amd64_windows_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* See if we can determine the end of the prologue via the symbol
      table.  If so, then return either the PC, or the PC after
      the prologue, whichever is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   return pc;
 }
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index f9feb52..a14ed41 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1384,22 +1384,16 @@  arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
   unsigned long inst;
-  CORE_ADDR skip_pc;
-  CORE_ADDR func_addr, limit_pc;
+  CORE_ADDR skip_pc, limit_pc, post_prologue_pc;
 
-  /* See if we can determine the end of the prologue via the symbol table.
-     If so, then return either PC, or the PC after the prologue, whichever
-     is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+  /* See if we can determine the end of the prologue via the symbol table.  */
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
     {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      struct symtab *s = find_pc_symtab (func_addr);
-
-      if (post_prologue_pc)
-	post_prologue_pc
-	  = arm_skip_stack_protector (post_prologue_pc, gdbarch);
+      struct symtab *s = find_pc_symtab (pc);
+      CORE_ADDR analyzed_limit;
 
+      post_prologue_pc = arm_skip_stack_protector (post_prologue_pc, gdbarch);
 
       /* GCC always emits a line note before the prologue and another
 	 one after, even if the two are at the same address or on the
@@ -1407,38 +1401,32 @@  arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 	 know every instruction that might appear in the prologue.  We
 	 will have producer information for most binaries; if it is
 	 missing (e.g. for -gstabs), assuming the GNU tools.  */
-      if (post_prologue_pc
-	  && (s == NULL
-	      || s->producer == NULL
-	      || strncmp (s->producer, "GNU ", sizeof ("GNU ") - 1) == 0 
-	      || strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
+      if (s == NULL
+	  || s->producer == NULL
+	  || strncmp (s->producer, "GNU ", sizeof ("GNU ") - 1) == 0 
+	  || strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0)
 	return post_prologue_pc;
 
-      if (post_prologue_pc != 0)
-	{
-	  CORE_ADDR analyzed_limit;
-
-	  /* For non-GCC compilers, make sure the entire line is an
-	     acceptable prologue; GDB will round this function's
-	     return value up to the end of the following line so we
-	     can not skip just part of a line (and we do not want to).
+      /* For non-GCC compilers, make sure the entire line is an
+	 acceptable prologue; GDB will round this function's
+	 return value up to the end of the following line so we
+	 can not skip just part of a line (and we do not want to).
 
-	     RealView does not treat the prologue specially, but does
-	     associate prologue code with the opening brace; so this
-	     lets us skip the first line if we think it is the opening
-	     brace.  */
-	  if (arm_pc_is_thumb (gdbarch, func_addr))
-	    analyzed_limit = thumb_analyze_prologue (gdbarch, func_addr,
-						     post_prologue_pc, NULL);
-	  else
-	    analyzed_limit = arm_analyze_prologue (gdbarch, func_addr,
-						   post_prologue_pc, NULL);
+	 RealView does not treat the prologue specially, but does
+	 associate prologue code with the opening brace; so this
+	 lets us skip the first line if we think it is the opening
+	 brace.  */
+      if (arm_pc_is_thumb (gdbarch, pc))
+	analyzed_limit = thumb_analyze_prologue (gdbarch, pc,
+						 post_prologue_pc, NULL);
+      else
+	analyzed_limit = arm_analyze_prologue (gdbarch, pc,
+					       post_prologue_pc, NULL);
 
-	  if (analyzed_limit != post_prologue_pc)
-	    return func_addr;
+      if (analyzed_limit != post_prologue_pc)
+	return pc;
 
-	  return post_prologue_pc;
-	}
+      return post_prologue_pc;
     }
 
   /* Can't determine prologue from the symbol table, need to examine
diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index d2b5f5f..c460959 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -874,15 +874,14 @@  avr_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   CORE_ADDR func_addr, func_end;
   CORE_ADDR post_prologue_pc;
 
-  /* See what the symbol table says */
+  /* See what the symbol table says.  */
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   if (!find_pc_partial_function (pc, NULL, &func_addr, &func_end))
     return pc;
 
-  post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr);
-  if (post_prologue_pc != 0)
-    return max (pc, post_prologue_pc);
-
   {
     CORE_ADDR prologue_end = pc;
     struct avr_unwind_cache info = {0};
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 1e68505..02e3eaa 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1812,20 +1812,18 @@  i386_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
   CORE_ADDR pc;
   gdb_byte op;
   int i;
-  CORE_ADDR func_addr;
+  CORE_ADDR post_prologue_pc;
 
-  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (post_prologue_pc != 0)
     {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      struct symtab *s = find_pc_symtab (func_addr);
+      struct symtab *s = find_pc_symtab (start_pc);
 
       /* Clang always emits a line note before the prologue and another
 	 one after.  We trust clang to emit usable line notes.  */
-      if (post_prologue_pc
-	  && (s != NULL
-	      && s->producer != NULL
-	      && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
+      if (s != NULL
+	  && s->producer != NULL
+	  && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0)
         return max (start_pc, post_prologue_pc);
     }
  
diff --git a/gdb/lm32-tdep.c b/gdb/lm32-tdep.c
index 908abce..7045d2f 100644
--- a/gdb/lm32-tdep.c
+++ b/gdb/lm32-tdep.c
@@ -185,20 +185,16 @@  lm32_analyze_prologue (struct gdbarch *gdbarch,
 static CORE_ADDR
 lm32_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
-  CORE_ADDR func_addr, limit_pc;
+  CORE_ADDR post_prologue_pc, limit_pc;
   struct lm32_frame_cache frame_info;
   struct trad_frame_saved_reg saved_regs[SIM_LM32_NUM_REGS];
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
@@ -206,9 +202,8 @@  lm32_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* Find an upper limit on the function prologue using the debug
      information.  If the debug information could not be used to provide
      that bound, then use an arbitrary large number as the upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    limit_pc = pc + 100;	/* Magic.  */
+  limit_pc = pc + 100;	/* Magic.  */
+  /* FIXME: could do what rs6000-tdep.c does here */
 
   frame_info.saved_regs = saved_regs;
   return lm32_analyze_prologue (gdbarch, pc, limit_pc, &frame_info);
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 188580f..278ccfe 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -6398,18 +6398,14 @@  static CORE_ADDR
 mips_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   CORE_ADDR limit_pc;
-  CORE_ADDR func_addr;
+  CORE_ADDR post_prologue_pc;
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
@@ -6417,9 +6413,8 @@  mips_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* Find an upper limit on the function prologue using the debug
      information.  If the debug information could not be used to provide
      that bound, then use an arbitrary large number as the upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    limit_pc = pc + 100;          /* Magic.  */
+  limit_pc = pc + 100;          /* Magic.  */
+  /* FIXME: could do what rs6000-tdep.c does here */
 
   if (mips_pc_is_mips16 (gdbarch, pc))
     return mips16_scan_prologue (gdbarch, pc, limit_pc, NULL, NULL);
diff --git a/gdb/moxie-tdep.c b/gdb/moxie-tdep.c
index 7926927..8b1fdfd 100644
--- a/gdb/moxie-tdep.c
+++ b/gdb/moxie-tdep.c
@@ -215,49 +215,43 @@  moxie_analyze_prologue (CORE_ADDR start_addr, CORE_ADDR end_addr,
 static CORE_ADDR
 moxie_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
-  CORE_ADDR func_addr = 0, func_end = 0;
+  CORE_ADDR func_addr, func_end;
+  CORE_ADDR post_prologue_pc;
   const char *func_name;
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
+
   if (find_pc_partial_function (pc, &func_name, &func_addr, &func_end))
     {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-      else
+      /* Can't determine prologue from the symbol table, need to examine
+	 instructions.  */
+      struct symtab_and_line sal;
+      struct symbol *sym;
+      struct moxie_frame_cache cache;
+      CORE_ADDR plg_end;
+
+      memset (&cache, 0, sizeof cache);
+
+      plg_end = moxie_analyze_prologue (func_addr, func_end, &cache, gdbarch);
+      /* Found a function.  */
+      sym = lookup_symbol (func_name, NULL, VAR_DOMAIN, NULL);
+      /* Don't use line number debug info for assembly source files.  */
+      if (sym && SYMBOL_LANGUAGE (sym) != language_asm)
 	{
-	  /* Can't determine prologue from the symbol table, need to examine
-	     instructions.  */
-	  struct symtab_and_line sal;
-	  struct symbol *sym;
-	  struct moxie_frame_cache cache;
-	  CORE_ADDR plg_end;
-	  
-	  memset (&cache, 0, sizeof cache);
-	  
-	  plg_end = moxie_analyze_prologue (func_addr, 
-					    func_end, &cache, gdbarch);
-	  /* Found a function.  */
-	  sym = lookup_symbol (func_name, NULL, VAR_DOMAIN, NULL);
-	  /* Don't use line number debug info for assembly source
-	     files.  */
-	  if (sym && SYMBOL_LANGUAGE (sym) != language_asm)
+	  sal = find_pc_line (func_addr, 0);
+	  if (sal.end && sal.end < func_end)
 	    {
-	      sal = find_pc_line (func_addr, 0);
-	      if (sal.end && sal.end < func_end)
-		{
-		  /* Found a line number, use it as end of
-		     prologue.  */
-		  return sal.end;
-		}
+	      /* Found a line number, use it as end of prologue.  */
+	      return sal.end;
 	    }
-	  /* No useable line symbol.  Use result of prologue parsing
-	     method.  */
-	  return plg_end;
 	}
+      /* No useable line symbol.  Use result of prologue parsing method.  */
+      return plg_end;
     }
 
   /* No function symbol -- just return the PC.  */
diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c
index 18a5913..b7ffa1d 100644
--- a/gdb/nios2-tdep.c
+++ b/gdb/nios2-tdep.c
@@ -945,20 +945,15 @@  nios2_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
   CORE_ADDR limit_pc;
   CORE_ADDR func_addr;
-
+  CORE_ADDR post_prologue_pc;
   struct nios2_unwind_cache cache;
 
   /* See if we can determine the end of the prologue via the symbol
      table.  If so, then return either PC, or the PC after the
      prologue, whichever is greater.  */
-  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-        = skip_prologue_using_sal (gdbarch, func_addr);
-
-      if (post_prologue_pc != 0)
-        return max (start_pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (post_prologue_pc != 0)
+    return max (start_pc, post_prologue_pc);
 
   /* Prologue analysis does the rest....  */
   nios2_init_cache (&cache, start_pc);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 730afe7..94c9908 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2108,33 +2108,29 @@  static CORE_ADDR
 rs6000_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   struct rs6000_framedata frame;
-  CORE_ADDR limit_pc, func_addr, func_end_addr = 0;
+  CORE_ADDR limit_pc, func_addr, func_end_addr;
+  CORE_ADDR post_prologue_pc;
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-	return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
 
-  /* Find an upper limit on the function prologue using the debug
-     information.  If the debug information could not be used to provide
-     that bound, then use an arbitrary large number as the upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    limit_pc = pc + 100;          /* Magic.  */
+  /* Use an arbitrary large number as the upper bound.  */
+  limit_pc = pc + 100;          /* Magic.  */
 
   /* Do not allow limit_pc to be past the function end, if we know
      where that end is...  */
-  if (func_end_addr && limit_pc > func_end_addr)
-    limit_pc = func_end_addr;
+  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
+    {
+      if (func_end_addr && limit_pc > func_end_addr)
+	limit_pc = func_end_addr;
+    }
 
   pc = skip_prologue (gdbarch, pc, limit_pc, &frame);
   return pc;
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index c8c36db..0124062 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -724,12 +724,9 @@  sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
-    {
-      post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-        return max (pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, pc);
+  if (post_prologue_pc != 0)
+    return max (pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
@@ -737,15 +734,16 @@  sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* Find an upper limit on the function prologue using the debug
      information.  If the debug information could not be used to provide
      that bound, then use an arbitrary large number as the upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    /* Don't go any further than 28 instructions.  */
-    limit_pc = pc + (2 * 28);
+  /* Don't go any further than 28 instructions.  */
+  limit_pc = pc + (2 * 28);
 
   /* Do not allow limit_pc to be past the function end, if we know
      where that end is...  */
-  if (func_end_addr != 0)
-    limit_pc = min (limit_pc, func_end_addr);
+  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
+    {
+      if (func_end_addr && limit_pc > func_end_addr)
+	limit_pc = func_end_addr;
+    }
 
   cache.sp_offset = -4;
   post_prologue_pc = sh_analyze_prologue (gdbarch, pc, limit_pc, &cache, 0);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index c530d50..bdaa81a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3107,7 +3107,7 @@  in_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR func_start)
   return func_addr <= pc && pc < sal.end;
 }
 
-/* Given PC at the function's start address, attempt to find the
+/* Given FUNC_ADDR, the function's start address, attempt to find the
    prologue end using SAL information.  Return zero if the skip fails.
 
    A non-optimized prologue traditionally has one SAL for the function
@@ -3133,7 +3133,8 @@  skip_prologue_using_sal (struct gdbarch *gdbarch, CORE_ADDR func_addr)
   const struct block *bl;
 
   /* Get an initial range for the function.  */
-  find_pc_partial_function (func_addr, NULL, &start_pc, &end_pc);
+  if (!find_pc_partial_function (func_addr, NULL, &start_pc, &end_pc))
+    return 0;
   start_pc += gdbarch_deprecated_function_start_offset (gdbarch);
 
   prologue_sal = find_pc_line (start_pc, 0);
diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
index ae3138e..eaa50eb 100644
--- a/gdb/tic6x-tdep.c
+++ b/gdb/tic6x-tdep.c
@@ -300,19 +300,15 @@  tic6x_analyze_prologue (struct gdbarch *gdbarch, const CORE_ADDR start_pc,
 static CORE_ADDR
 tic6x_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
-  CORE_ADDR func_addr;
+  CORE_ADDR post_prologue_pc;
   struct tic6x_unwind_cache cache;
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever is
      greater.  */
-  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-	= skip_prologue_using_sal (gdbarch, func_addr);
-      if (post_prologue_pc != 0)
-	return max (start_pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (post_prologue_pc != 0)
+    return max (start_pc, post_prologue_pc);
 
   /* Can't determine prologue from the symbol table, need to examine
      instructions.  */
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index befab56..d8b9c89 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -744,19 +744,14 @@  tilegx_analyze_prologue (struct gdbarch* gdbarch,
 static CORE_ADDR
 tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
-  CORE_ADDR func_start, end_pc;
+  CORE_ADDR post_prologue_pc, end_pc;
   struct obj_section *s;
 
   /* This is the preferred method, find the end of the prologue by
      using the debugging information.  */
-  if (find_pc_partial_function (start_pc, NULL, &func_start, NULL))
-    {
-      CORE_ADDR post_prologue_pc
-        = skip_prologue_using_sal (gdbarch, func_start);
-
-      if (post_prologue_pc != 0)
-        return max (start_pc, post_prologue_pc);
-    }
+  post_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (post_prologue_pc != 0)
+    return max (start_pc, post_prologue_pc);
 
   /* Don't straddle a section boundary.  */
   s = find_pc_section (start_pc);