Re: [patch] Do not skip prologue for asm (.S) files

[Doug Evans <dje at google dot com>]

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.

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))
     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.
 >