diff mbox

[patch+7.12.1,1/2] Code cleanup: write_exp_msymbol: +is_tls

Message ID 20161009185641.GA13645@host1.jankratochvil.net
State New, archived
Headers

Commit Message

Jan Kratochvil Oct. 9, 2016, 6:56 p.m. UTC 
  Hi,

no functionality change, for patch 2/2.


Jan
gdb/ChangeLog
2016-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* parse.c (write_exp_msymbol): New variable is_tls, use it.

Comments

Yao Qi Oct. 11, 2016, 2:21 p.m. UTC | #1 
On Sun, Oct 9, 2016 at 7:56 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
>
>    /* 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, addr, &current_target);
> -  if (pc != addr)
> +  if (!is_tls && pc != addr)

It does have functionality change.  This should be moved to patch #2?
Jan Kratochvil Oct. 11, 2016, 2:27 p.m. UTC | #2 
On Tue, 11 Oct 2016 16:21:58 +0200, Yao Qi wrote:
> On Sun, Oct 9, 2016 at 7:56 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
> > --- a/gdb/parse.c
> > +++ b/gdb/parse.c
> >
> >    /* 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, addr, &current_target);
> > -  if (pc != addr)
> > +  if (!is_tls && pc != addr)
> 
> It does have functionality change.

Ah, you are right.


> This should be moved to patch #2?

It is unrelated to patch #2, it could be a new patch #3 (or patch #1.5).
In fact if (tls) then gdbarch_convert_from_func_ptr_addr should not be even
called so this patch is also wrong (suboptimal).

Let's forget about this patch hunk now.


Jan
Pedro Alves Sept. 6, 2017, 12:14 p.m. UTC | #3 
On 10/09/2016 07:56 PM, Jan Kratochvil wrote:
> Hi,
> 
> no functionality change, for patch 2/2.


>    /* 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, addr, &current_target);
> -  if (pc != addr)
> +  if (!is_tls && pc != addr)

This bit might be a functionality change / fix, AFAICS.  I pushed in
the  other patch without this bit, because:

 - it looked like we could skip the gdbarch_convert_from_func_ptr_addr
   call too?

 - I wondered whether for TLS function pointers we should
   actually translate the TLS address and then
   still see if the translated address points to a function
   descriptor that needs to be resolved, like in the non-TLS case.

At this point, it seemed better to leave it be until we add
a testcase with a TLS function pointer.

Thanks,
Pedro Alves
diff mbox

Patch

--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -484,11 +484,13 @@  write_exp_msymbol (struct parser_state *ps,
   struct obj_section *section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
   enum minimal_symbol_type type = MSYMBOL_TYPE (msymbol);
   CORE_ADDR pc;
+  const int is_tls = (section != NULL
+		      && section->the_bfd_section->flags & SEC_THREAD_LOCAL);
 
   /* 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, addr, &current_target);
-  if (pc != addr)
+  if (!is_tls && pc != addr)
     {
       struct bound_minimal_symbol ifunc_msym = lookup_minimal_symbol_by_pc (pc);
 
@@ -520,7 +522,7 @@  write_exp_msymbol (struct parser_state *ps,
   write_exp_elt_longcst (ps, (LONGEST) addr);
   write_exp_elt_opcode (ps, OP_LONG);
 
-  if (section && section->the_bfd_section->flags & SEC_THREAD_LOCAL)
+  if (is_tls)
     {
       write_exp_elt_opcode (ps, UNOP_MEMVAL_TLS);
       write_exp_elt_objfile (ps, objfile);