[RFC] Support command "catch syscall" properly on different targets
Commit Message
On Friday, March 06 2015, Yao Qi wrote:
> In my patch, "catch syscall" command is allowed on the target which
> supports catch syscall. On exec target, GDB will error on command
> "catch syscall", so it causes regressions in catch-syscall.exp.
Hey Yao,
So, as I said in my previous messages, I don't think that making the
"catch syscall" command to fail on the exec target.
What do you think of the attached patch (applies on top of your patch,
rebased to the current HEAD)? It implements what I proposed, but in a
different way. If the target is "None" (no binary loaded) or "exec"
(inferior loaded but never started), then it displays a warning but
still creates the catchpoint. The actual check for these targets
happens in the insert_catch_syscall function, which is called when we
already know if the target actually supports the syscall catchpoint.
Maybe I forgot to cover some corner case, but I still think we should
support "catch syscall" when no inferior has been started.
> The proc test_catch_syscall_multi_arch in catch-syscall.exp tests
> syscall number is mapped to different syscall number on different
> gdbarch, but it has:
>
> # We are not interested in loading any binary here, and in
> # some systems (PowerPC, for example), if we load a binary
> # there is no way to set other architecture.
>
> As you mentioned in
> https://sourceware.org/bugzilla/show_bug.cgi?id=10737#c4 , when GDB
> starts an inferior, the arch is set correspondingly, so it is unable
> reproduce the bug. test_catch_syscall_multi_arch is the simpler
> reproducer, IMO. How about 'upgrade' test_catch_syscall_multi_arch like
> this?
>
> - file 32-bit program
> - run to main
> - catch syscall 5
> - kill
> - file 64-bit program
> - run to main
> - catch syscall 5
It would work (though, by testing this on PPC64, I still see the strange
warnings about changing to an unknown architecture, even after compiling
with targets=all).
With the attached patch, we would only need to expect the message saying
that the syscall catchpoint may not be supported in the target, and
still would not need a binary to perform the test.
WDYT?
Comments
Sergio Durigan Junior <sergiodj@redhat.com> writes:
> So, as I said in my previous messages, I don't think that making the
> "catch syscall" command to fail on the exec target.
>
> What do you think of the attached patch (applies on top of your patch,
> rebased to the current HEAD)? It implements what I proposed, but in a
> different way. If the target is "None" (no binary loaded) or "exec"
> (inferior loaded but never started), then it displays a warning but
> still creates the catchpoint. The actual check for these targets
> happens in the insert_catch_syscall function, which is called when we
> already know if the target actually supports the syscall catchpoint.
>
> Maybe I forgot to cover some corner case, but I still think we should
> support "catch syscall" when no inferior has been started.
I don't have a strong opinion against your approach. Since "catch
point" is only supported on some arches on linux native target, I think
it is OK to leave gdbarch_get_syscall_number_p checking in
catch_syscall_command_1, so I withdraw my patch.
However, when I play with your patch, I find GDB can disable catch point if it
isn't inserted successfully, in breakpoint.c:insert_bp_location,
else if (bl->owner->type == bp_catchpoint)
{
int val;
gdb_assert (bl->owner->ops != NULL
&& bl->owner->ops->insert_location != NULL);
val = bl->owner->ops->insert_location (bl);
if (val)
{
bl->owner->enable_state = bp_disabled;
if (val == 1)
warning (_("\
Error inserting catchpoint %d: Your system does not support this type\n\
of catchpoint."), bl->owner->number);
else
warning (_("Error inserting catchpoint %d."), bl->owner->number);
}
as shown below,
(gdb) target remote :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
0x00007ffff7ddb2d0 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) c
Continuing.
warning: Error inserting catchpoint 1: Your system does not support this type
of catchpoint.
....
(gdb) info breakpoints
Num Type Disp Enb Address What
1 catchpoint keep n syscall "open"
According this observation, I don't see the need check
gdbarch_get_syscall_number_p in catch_syscall_command_1. Probably we
can remove it.
On Monday, March 09 2015, Yao Qi wrote:
> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> So, as I said in my previous messages, I don't think that making the
>> "catch syscall" command to fail on the exec target.
>>
>> What do you think of the attached patch (applies on top of your patch,
>> rebased to the current HEAD)? It implements what I proposed, but in a
>> different way. If the target is "None" (no binary loaded) or "exec"
>> (inferior loaded but never started), then it displays a warning but
>> still creates the catchpoint. The actual check for these targets
>> happens in the insert_catch_syscall function, which is called when we
>> already know if the target actually supports the syscall catchpoint.
>>
>> Maybe I forgot to cover some corner case, but I still think we should
>> support "catch syscall" when no inferior has been started.
>
> I don't have a strong opinion against your approach. Since "catch
> point" is only supported on some arches on linux native target, I think
> it is OK to leave gdbarch_get_syscall_number_p checking in
> catch_syscall_command_1, so I withdraw my patch.
Right. If you withdraw your patch, then my patch doesn't make sense
:-).
> However, when I play with your patch, I find GDB can disable catch point if it
> isn't inserted successfully, in breakpoint.c:insert_bp_location,
>
> else if (bl->owner->type == bp_catchpoint)
> {
> int val;
>
> gdb_assert (bl->owner->ops != NULL
> && bl->owner->ops->insert_location != NULL);
>
> val = bl->owner->ops->insert_location (bl);
> if (val)
> {
> bl->owner->enable_state = bp_disabled;
>
> if (val == 1)
> warning (_("\
> Error inserting catchpoint %d: Your system does not support this type\n\
> of catchpoint."), bl->owner->number);
> else
> warning (_("Error inserting catchpoint %d."), bl->owner->number);
> }
Yeah, this is the reason my patch was returning 1 on
insert_catch_syscall.
The good thing about this is that GDB also keeps the record for this
type of catchpoint, so that the warning isn't repeated over and over.
But you probably knew there :-).
> as shown below,
>
> (gdb) target remote :1234
> Remote debugging using :1234
> Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
> 0x00007ffff7ddb2d0 in ?? () from /lib64/ld-linux-x86-64.so.2
> (gdb) c
> Continuing.
> warning: Error inserting catchpoint 1: Your system does not support this type
> of catchpoint.
> ....
>
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 catchpoint keep n syscall "open"
>
> According this observation, I don't see the need check
> gdbarch_get_syscall_number_p in catch_syscall_command_1. Probably we
> can remove it.
Yeah, it can be removed. This will make the 'catch syscall' command
more similar to the other catchpoint commands, although I liked the idea
of your patch...
Cheers,
===================================================================
@@ -8517,6 +8517,15 @@ insert_catch_syscall (struct bp_location
struct catch_syscall_inferior_data *inf_data
= get_catch_syscall_inferior_data (inf);
+ /* Checking if the target supports 'catch syscall'. Here we should
+ already know the definitive answer. */
+ if (!target_supports_syscall_catchpoint (get_current_arch ()))
+ {
+ warning (_("The feature 'catch syscall' is not supported on \
+this target."));
+ return 1;
+ }
+
++inf_data->total_syscalls_count;
if (!c->syscalls_to_be_caught)
++inf_data->any_syscall_count;
@@ -12168,8 +12177,20 @@ catch_syscall_command_1 (char *arg, int
/* Checking if the feature if supported. */
if (!target_supports_syscall_catchpoint (gdbarch))
- error (_("The feature 'catch syscall' is not supported on \
+ {
+ /* If our target is "None" (i.e., no binary loaded), or "exec"
+ (i.e., binary has been loaded, but the inferior has not been
+ started yet), then we still have no way to know if our target
+ supports syscall catchpoints. Therefore, we just warn the
+ user, but keep going. */
+ if (strcmp (target_shortname, "None") == 0
+ || strcmp (target_shortname, "exec") == 0)
+ warning (_("The feature 'catch syscall' may not be supported \
+on this target."));
+ else
+ error (_("The feature 'catch syscall' is not supported on \
this target yet."));
+ }
tempflag = get_cmd_context (command) == CATCH_TEMPORARY;