diff mbox

Do not skip prologue for asm (.S) files

Message ID 047d7bfeb318a3caf305195d7da8@google.com
State New
Headers show

Commit Message

Doug Evans June 25, 2015, 8:30 p.m. UTC 
Jan Kratochvil writes:
  > On Tue, 23 Jun 2015 22:35:01 +0200, Jan Kratochvil wrote:
  > > On Tue, 23 Jun 2015 01:46:08 +0200, Doug Evans wrote:
  > > > static void
  > > > minsym_found (struct linespec_state *self, struct objfile *objfile,
  > > >               struct minimal_symbol *msymbol,
  > > >               struct symtabs_and_lines *result)
  > > > {
  > > >   struct gdbarch *gdbarch = get_objfile_arch (objfile);
  > > >   CORE_ADDR pc;
  > > >   struct symtab_and_line sal;
  > > >
  > > >   sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
  > > >                            (struct obj_section *) 0, 0);
  > > >   sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
  > > >
  > > >   /* The minimal symbol might point to a function descriptor;
  > > >      resolve it to the actual code address instead.  */
  > > >   pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,  
&current_target);
  > > >   if (pc != sal.pc)
  > > >     sal = find_pc_sect_line (pc, NULL, 0);
  > > >
  > > >   if (self->funfirstline)
  > > >     skip_prologue_sal (&sal);
  > > >
  > > >   if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
  > > >     add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME  
(msymbol), 0);
  > > > }
  > > >
  > > > With the patch added, we're using the value of
  > > > MSYMBOL_VALUE_ADDRESS twice
  > > > and calling gdbarch_convert_from_func_ptr_addr twice.
  > > > [I'm not micro-optimizing here, my goal is code readability.]
  > > >
  > > > Plus the patch does:
  > > >
  > > >       sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
  > > >       sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
  > > >                                                   &current_target);
  > > >
  > > > but it doesn't update sal.section nor sal.line.
  > >
  > > OK, I agree that seems wrong.
  >
  > I do not agree, it seems correct to me.

I was wondering if things were correct, not stating they weren't.

  > I have only added a comment to the code.  Is it enough this way?

The comment helps, thanks.
I'm still uncomfortable with setting locations_valid for assembler.
The flag may get used for more things in the future, and this feels
like asking for trouble.
Fortunately, I think there's a better way: instead of setting  
locations_valid
check the language at the place where we care.

      add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol),  
0);


[and leave dwarf2read.c as is]

Ok with this change.
Thanks!

  > I am sorry I cannot write it much cleanly as the data structures and  
functions
  > involved are not much clean.
  >
  >
  > Jan
  > gdb/ChangeLog
  > 2015-06-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
  >
  > 	* dwarf2read.c (process_full_comp_unit): Set LOCATIONS_VALID also for
  > 	language_asm.
  > 	* linespec.c (minsym_found): Reset sal.PC forCOMPUNIT_LOCATIONS_VALID.
  >
  > gdb/testsuite/ChangeLog
  > 2015-06-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
  >
  > 	* gdb.arch/amd64-prologue-skip.S: New file.
  > 	* gdb.arch/amd64-prologue-skip.exp: New file.
  >

Comments

Jan Kratochvil June 25, 2015, 8:37 p.m. UTC | #1 
On Thu, 25 Jun 2015 22:30:14 +0200, Doug Evans wrote:
> Jan Kratochvil writes:
>  > On Tue, 23 Jun 2015 22:35:01 +0200, Jan Kratochvil wrote:
>  > > On Tue, 23 Jun 2015 01:46:08 +0200, Doug Evans wrote:
>  > > > but it doesn't update sal.section nor sal.line.
>  > >
>  > > OK, I agree that seems wrong.
>  >
>  > I do not agree, it seems correct to me.
> 
> I was wondering if things were correct, not stating they weren't.

Fine with that, just you have convinced me initially there is really a bug.


> Fortunately, I think there's a better way: instead of setting
> locations_valid check the language at the place where we care.

I am sure fine with that.

> Ok with this change.

Just there is now also [patchv2].


Thanks,
Jan
diff mbox

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index d2089b5..71bab61 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -3454,7 +3454,23 @@  minsym_found (struct linespec_state *self, struct  
objfile *objfile,
      sal = find_pc_sect_line (pc, NULL, 0);

    if (self->funfirstline)
-    skip_prologue_sal (&sal);
+    {
+      if (sal.symtab != NULL
+	  && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
+	      || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
+	{
+	  /* If gdbarch_convert_from_func_ptr_addr does not apply then
+	     sal.SECTION, sal.LINE&co. will stay correct from above.
+	     If gdbarch_convert_from_func_ptr_addr applies then
+	     sal.SECTION is cleared from above and sal.LINE&co. will
+	     stay correct from the last find_pc_sect_line above.  */
+	  sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+	  sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
+						       &current_target);
+	}
+      else
+	skip_prologue_sal (&sal);
+    }

    if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))