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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 23 Jun 2015 01:46:08 +0200, Doug Evans wrote:
> If locations_valid and COMPUNIT_LOCATIONS_VALID
> were renamed to locations_valid_at_entry and
> COMPUNIT_LOCATIONS_VALID_AT_ENTRY
> this code would be massively more readable to me
> and might even be self documenting (and thus not needing
> any further comments, yay).
> 
> How about that?

"at entry" is only one of its possible use cases.  In fact it guarantees
"always", that is "for every PC".  That means not only "at entry" but also
"during prologue" and "during epilogue".  Although you are right the current
comment for "locations_valid" talks only about that "at entry" aspect.


> Here's the full function.
> [apologies if gmail messes this up :-(]

That is very OT but if some mailer messes up your mails why do you use it.


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


> Do we need to do anything at all here?
> IOW, what cases does the following alternative miss?
> I'm wondering if we need to add some clarity to
> find_pc_sect_line's API.
> 
>   if (self->funfirstline
>       && (sal.symtab == NULL
>             || !COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))))
>     skip_prologue_sal (&sal);

As I said SAL was created by find_pc_sect_line:
  sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
                           (struct obj_section *) 0, 0);
and it is not guaranteed that: sal.pc == MSYMBOL_VALUE_ADDRESS
even when we do not call skip_prologue_sal().
(ignoring gdbarch_convert_from_func_ptr_addr() here)


> But now I'm wondering how is the reader supposed to interpret the
> name "locations_valid". I can hear the reader asking
> "Locations are valid in assembler???"
> Renaming it to locations_valid_at_entry will help, a bit.

Discussed above.  Another solution is to rename it to "locations_not_valid",
that would be fine as 'false' for language_asm. :-)


> And comment saying that we're setting locations_valid_at_entry
> to avoid prologue skipping in the assembler case will improve the
> readability a lot (to this reader).
> 
> E.g.
> 
>   if ((cu->has_loclist && gcc_4_minor >= 5)
>       /* The goal here is to avoid prologue skipping, which we want
>           for assembler.  */
>       || cu->language == language_asm)

OK but we should first settle down on the "locations_valid" name.


Jan