Re: [PATCH] Don't look up agent symbols if it's off

[Yao Qi <qiyaoltc at gmail dot com>]

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.

-- 
Yao (éå)