This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 5/7 take 3] Improved linker-debugger interface
- From: Pedro Alves <palves at redhat dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>, gdb-patches at sourceware dot org
- Date: Thu, 30 May 2013 18:18:07 +0100
- Subject: Re: [RFA 5/7 take 3] Improved linker-debugger interface
- References: <20130516144340 dot GA2105 at blade dot nx> <20130516144838 dot GF2105 at blade dot nx> <20130517185002 dot GA10447 at host2 dot jankratochvil dot net> <20130524083044 dot GC4602 at blade dot nx> <51A64E24 dot 2020901 at redhat dot com> <20130530104346 dot GA6217 at blade dot nx>
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