| Message ID | 047d7bd9037cc3fa92051eb9bf4a@google.com |
|---|---|
| State | New |
| Headers | show |
Doug Evans <dje@google.com> writes: Hi Doug, > Looking at some agent code I see there's a bit of TLC needed here. > [E.g., target_can_use_agent isn't used anywhere.] > Most of the agent code was added by my patch series https://sourceware.org/ml/gdb-patches/2012-02/msg00344.html to teach both GDB and GDBserver talk with agent. I tested agent related code path by 1) setting agent on explicitly and 2) running gdb.trace/strace.exp with an very old ust library on my machine. > I didn't want to get bogged down in fixing all of it, > but I do want to address one issue: > > According to common/agent.c there are three symbols that are needed: > > IPA_SYM(helper_thread_id), > IPA_SYM(cmd_buf), > IPA_SYM(capability), > > but the default value of "set agent" is "off". > > (gdb) help set agent > Set debugger's willingness to use agent as a helper. > If on, GDB will delegate some of the debugging operations to the > agent, if the target supports it. This will speed up those > operations that are supported by the agent. > If off, GDB will not use agent, even if such is supported by the > target. > > Why are we looking up these symbols if the agent is off? > It's possible I'm missing something. "set agent on" is to let GDB delegate some debugging operations, like installing fast tracepoint, to agent, as an alternative. However, agent is still used in some operations if "set agent off", such as querying about the static tracepoint markers. > I can imagine that "agent" is ambiguous here as I see > linux-nat.c:linux_child_static_tracepoint_markers_by_strid > calling agent_run_command but I can't find a test that first checks > that that's ok (e.g., what if "set agent" is "off"?). > See the comments in tracepoint.c:info_static_tracepoint_markers_command, /* We don't have to check target_can_use_agent and agent's capability on static tracepoint here, in order to be compatible with older GDBserver. We don't check USE_AGENT is true or not, because static tracepoints don't work without in-process agent, so we don't bother users to type `set agent on' when to use static tracepoint. */ > This patch stops the symbol lookup if the agent is off, > and adds code to look them up when the agent is turned on. What is wrong with the symbol lookup if the agent is off? > But I think more is needed here. > E.g, what happens if linux_child_static_tracepoint_markers_by_strid > gets called but we haven't found the address of > gdb_agent_cmd_buf? > Perhaps that "can't happen", but where does one look to prove that? > > I ran the testsuite with --target_board=native-stdio-gdbserver, > but having dug a bit deeper I'm not sure it was useful. > The default for "set agent" is off and I can't > find any test that actually turns it on. So now I'm wondering > if some TLC needs to be applied to the testsuite too. :-( > E.g., does gdb.trace/ftrace.exp actually test what it claims to test? Yes, I think so. > I added a "set agent on" to ftrace.exp and got identical results, > but IWBN to have a way to verify the intended code got exercised. > E.g., I see tests for use_agent in gdbserver/tracepoint.c, > but I also see fallbacks in case the test fails. > Why aren't we testing with "set agent on"? GDBserver can download fast tracepoint through either writing inferior memory or sending agent command to delegate agent to do so. > > I also see pr 13808 is still with us. > setup_kfail "gdb/13808" "x86_64-*-linux*" > gdb_test "print globvar" " = 1" > We need to fix that, but it has nothing to do with agent. > btw, where do I get ust/marker.h? > It's needed by some tests but the packages that seem like they should > have it don't (e.g., lttng-ust{,-devel}). They are from old UST (0.11). In short, GDB UST static tracepoint marker is only usable with UST 0.11 and URCU 0.5.3. See https://www.sourceware.org/ml/gdb/2012-01/msg00026.html I think we need to remove GDB UST static tracepoint support.
diff --git a/gdb/agent.c b/gdb/agent.c index a5f6cdc..57f993d 100644 --- a/gdb/agent.c +++ b/gdb/agent.c @@ -47,8 +47,14 @@ static void set_can_use_agent (char *args, int from_tty, struct cmd_list_element *c) { if (target_use_agent (can_use_agent == can_use_agent_on) == 0) - /* Something wrong during setting, set flag to default value. */ + /* Something wrong during setting, agent is turned off. */ can_use_agent = can_use_agent_off; + + /* If we turned it on, and the support symbols haven't been loaded yet, + do it now. */ + if (can_use_agent == can_use_agent_on + && !agent_loaded_p ()) + agent_look_up_symbols (NULL); } /* -Wmissing-prototypes */ @@ -60,7 +66,9 @@ extern initialize_file_ftype _initialize_agent; static void agent_new_objfile (struct objfile *objfile) { - if (objfile == NULL || agent_loaded_p ()) + if (can_use_agent == can_use_agent_off + || objfile == NULL + || agent_loaded_p ()) return; agent_look_up_symbols (objfile); diff --git a/gdb/common/agent.c b/gdb/common/agent.c index 5bb18641..2e780e0 100644 --- a/gdb/common/agent.c +++ b/gdb/common/agent.c @@ -80,7 +80,9 @@ agent_loaded_p (void) } /* Look up all symbols needed by agent. Return 0 if all the symbols are - found, return non-zero otherwise. */ + found, return non-zero otherwise. + ARG is either an objfile that was just loaded, or NULL meaning to search + all objfiles. */ int agent_look_up_symbols (void *arg)
Hi. Looking at some agent code I see there's a bit of TLC needed here. [E.g., target_can_use_agent isn't used anywhere.] I didn't want to get bogged down in fixing all of it, but I do want to address one issue: According to common/agent.c there are three symbols that are needed: IPA_SYM(helper_thread_id), IPA_SYM(cmd_buf), IPA_SYM(capability), but the default value of "set agent" is "off". (gdb) help set agent Set debugger's willingness to use agent as a helper. If on, GDB will delegate some of the debugging operations to the agent, if the target supports it. This will speed up those operations that are supported by the agent. If off, GDB will not use agent, even if such is supported by the target. Why are we looking up these symbols if the agent is off? It's possible I'm missing something. I can imagine that "agent" is ambiguous here as I see linux-nat.c:linux_child_static_tracepoint_markers_by_strid calling agent_run_command but I can't find a test that first checks that that's ok (e.g., what if "set agent" is "off"?). This patch stops the symbol lookup if the agent is off, and adds code to look them up when the agent is turned on. But I think more is needed here. E.g, what happens if linux_child_static_tracepoint_markers_by_strid gets called but we haven't found the address of gdb_agent_cmd_buf? Perhaps that "can't happen", but where does one look to prove that? I ran the testsuite with --target_board=native-stdio-gdbserver, but having dug a bit deeper I'm not sure it was useful. The default for "set agent" is off and I can't find any test that actually turns it on. So now I'm wondering if some TLC needs to be applied to the testsuite too. :-( E.g., does gdb.trace/ftrace.exp actually test what it claims to test? I added a "set agent on" to ftrace.exp and got identical results, but IWBN to have a way to verify the intended code got exercised. E.g., I see tests for use_agent in gdbserver/tracepoint.c, but I also see fallbacks in case the test fails. Why aren't we testing with "set agent on"? I also see pr 13808 is still with us. setup_kfail "gdb/13808" "x86_64-*-linux*" gdb_test "print globvar" " = 1" btw, where do I get ust/marker.h? It's needed by some tests but the packages that seem like they should have it don't (e.g., lttng-ust{,-devel}). 2015-09-01 Doug Evans <dje@google.com> * agent.c (set_can_use_agent): If agent has been turned on, lookup agent symbols if necessary. (agent_new_objfile): Don't look up agent symbols if agent is off. * common/agent.h (agent_look_up_symbols): Improve comment.