Re: [RFA 5/7 take 3] Improved linker-debugger interface

[Pedro Alves <palves@redhat.com>]

On 05/30/2013 11:43 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 05/24/2013 09:30 AM, Gary Benson wrote:
>>> +static int
>>> +svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
>>> +{
>>> +  struct svr4_info *info = get_svr4_info ();
>>> +  struct bp_location *loc;
>>> +
>> ...
>>
>>> +  for (loc = b->loc; loc; loc = loc->next)
>>> +    {
>>> +      struct probe_and_action *pa = solib_event_probe_at (info, loc->address);
>>
>> There's no relation between INFO and the locations' inferior/program
>> space.  IOW, I believe this is doing the wrong thing for multi-process.
> 
> It took me a while to figure out what you meant here, but I think I
> see what you mean.  

Sorry.  :-/

For those following along at home, the issue is that
get_svr4_info () returns the svr4 state object corresponding
to the current program space, while here we're walking over
all locations of all breakpoints, including
breakpoints/locations of all inferiors.   So in
the 'solib_event_probe_at (info, loc->address)' call, we need
to pass it the 'info' of the program space of 'loc'.

> I've attached an updated patch with this function
> reworked.  Could you let me know if it looks correct?

...

+/* Helper function for svr4_update_solib_event_breakpoints.  */
+
+static int
+svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
+{
+  struct bp_location *loc;
+
+  if (b->type != bp_shlib_event)
+    {
+      /* Continue iterating.  */
+      return 0;
+    }
+
+  for (loc = b->loc; loc != NULL; loc = loc->next)
+    {
+      struct svr4_info *info;
+      struct probe_and_action *pa;
+
+      info = program_space_data (loc->pspace, solib_svr4_pspace_data);
+      if (info == NULL || info->probes_table == NULL)
+	continue;

Exactly.  Excellent, looks great now.

On 05/30/2013 11:43 AM, Gary Benson wrote:> +	    {
> +	      char name[32] = { '\0' };
> +
> +	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
> +		 shipped with an early version of the probes code in
> +		 which the probes' names were prefixed with "rtld_"
> +		 and the "map_failed" probe did not exist.  The
> +		 locations of the probes are otherwise the same, so
> +		 we check for probes with prefixed names if probes
> +		 with unprefixed names are not present.  */
> +
> +	      if (with_prefix)
> +		strncat (name, "rtld_", sizeof (name) - strlen (name) - 1);
> +
> +	      strncat (name, probe_info[i].name,
> +		       sizeof (name) - strlen (name) - 1);
> +

BTW, I'd rather this was written as something like:

	      char name[32];

	      if (with_prefix)
		xsnprintf (name, sizeof (name), "rtld_%s", probe_info[i].name);
	      else
		xsnprintf (name, sizeof (name), "%s", probe_info[i].name);

strncat like that is really not future proof, as we'll just get get a silently
truncated string if a new probe appears with a name that is too big.
xsnprintf OTOH gdb_asserts the string fits in the buffer.  Granted, whoever 
added such a probe would probably notice this, but it's a style best avoided.
(Witness how so few strncat calls there are in gdb.  There are more strcat calls,
but that only shows that whenever we _do_ care about string overruns, we tend
to use something else, not strncat.)
Plus, it looks more readable to me that way.  :-)
Alternatively, we could add a xstrncat that asserts truncation never happens,
though xsnprintf in this case still looks a bit more readable to me :-)

-- 
Pedro Alves