Re: [PATCH 2/3] gdb/jit: enable tracking multiple jitter objfiles

[Simon Marchi <simark@simark.ca>]

On 2020-05-25 5:38 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> diff --git a/gdb/jit.c b/gdb/jit.c
> index fdb1248ed5b..c689ac3f392 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -966,58 +966,53 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,

The comment above jit_breakpoint_re_set_internal says:

  Return 0 if the jit breakpoint has been successfully initialized.

Can you adjust it to the new reality?  Something like "Return 0 if one
or more JIT breakpoints were successfully initialized".

I think it would also be good (as a separate cleanup) to make this function
return a bool, returning true when one or more JIT breakpoints were inserted.

> @@ -1357,38 +1352,38 @@ jit_event_handler (struct gdbarch *gdbarch)
>  
>    jit_program_space_data *ps_data = get_jit_program_space_data ();
>  
> -  /* Fetch the saved objfile.  */
> -  objfile *objf = nullptr;
> -  if (!ps_data->objfile_and_bps.empty ())
> -    objf = (ps_data->objfile_and_bps.begin ())->first;
> -
> -  /* Read the descriptor from remote memory.  */
> -  if (!jit_read_descriptor (gdbarch, &descriptor, objf))
> -    return;
> -  entry_addr = descriptor.relevant_entry;
> -
> -  /* Do the corresponding action.  */
> -  switch (descriptor.action_flag)
> +  for (auto &objf_and_bp : ps_data->objfile_and_bps)
>      {
> -    case JIT_NOACTION:
> -      break;
> -    case JIT_REGISTER:
> -      jit_read_code_entry (gdbarch, entry_addr, &code_entry);
> -      jit_register_code (gdbarch, entry_addr, &code_entry);
> -      break;
> -    case JIT_UNREGISTER:
> -      objf = jit_find_objf_with_entry_addr (entry_addr);
> -      if (objf == NULL)
> -	printf_unfiltered (_("Unable to find JITed code "
> -			     "entry at address: %s\n"),
> -			   paddress (gdbarch, entry_addr));
> -      else
> -	objf->unlink ();
> +      objfile *objf = objf_and_bp.first;
>  
> -      break;
> -    default:
> -      error (_("Unknown action_flag value in JIT descriptor!"));
> -      break;
> +      /* Read the descriptor from remote memory.  */
> +      if (!jit_read_descriptor (gdbarch, &descriptor, objf))
> +	continue;
> +      entry_addr = descriptor.relevant_entry;
> +
> +      /* Do the corresponding action.  */
> +      switch (descriptor.action_flag)
> +	{
> +	case JIT_NOACTION:
> +	  break;
> +	case JIT_REGISTER:
> +	  jit_read_code_entry (gdbarch, entry_addr, &code_entry);
> +	  jit_register_code (gdbarch, entry_addr, &code_entry);
> +	  break;
> +	case JIT_UNREGISTER:
> +	  objf = jit_find_objf_with_entry_addr (entry_addr);
> +	  if (objf == NULL)
> +	    printf_unfiltered (_("Unable to find JITed code "
> +				 "entry at address: %s\n"),
> +			       paddress (gdbarch, entry_addr));
> +	  else
> +	    objf->unlink ();
> +
> +	  break;
> +	default:
> +	  error (_("Unknown action_flag value in JIT descriptor!"));
> +	  break;
> +	}

I don't think that iterating on all JIT-providing objfiles is the correct thing
to do here.  After all, we have hit the __jit_debug_register_code breakpoint of
exactly one of them, so we should only process the jit descriptor of that one.
At the same moment, the other JIT-providing objfiles could have prepared anything
in their descriptor (or there could be junk data), but they have not asked us
to process it.

I haven't tried, but I bet that you could build a test to trigger a bug here:

1. Have two jit-providing objfiles register one jit objfile each
2. Have the two jit-providing objfiles prepare their descriptor with JIT_UNREGISTER
   and the address of their jit objfile
3. Have one of them call __jit_debug_register_code.

We would forget about both jit objfiles, when in reality we should forget only about
one.

I think we'll need to know which jit event breakpoint was hit, so we can know which
jit-providing objfile asked us to process its descriptor.

Simon