[PATCH 4/7] btrace: Add support for interrupt events.
Willgerodt, Felix
felix.willgerodt@intel.com
Wed Sep 11 14:49:05 GMT 2024
> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 10. September 2024 12:48
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH 4/7] btrace: Add support for interrupt events.
>
> Hello Felix,
>
> >+ case BTRACE_INSN_AUX:
> >+ /* An aux insn couldn't have switched the function. But the
> >+ segment might not have had a symbol name resolved yet, as events
> >+ might not have an IP. Use the current IP in that case and update
> >+ the name. */
> >+ if (bfun->sym == nullptr && bfun->msym == nullptr)
> >+ {
> >+ bfun->sym = fun;
> >+ bfun->msym = mfun;
> >+ }
> >+ return bfun;
>
> We shouldn't normally switch functions just because we turn on event tracing.
> Since an event may or may not provide an IP, however, there is the possibility
> that two adjacent events belong to different functions.
I don't really understand this scenario. Are you talking about:
Last instruction Function 1 - event (function 1) - event (function 2) - first instruction function 2
How can that actually happen?
Aren't events always bound to the instruction they arrive at (if they have no IP)?
Can events really arrive between two instructions? And even then, isn't the event
then automatically bound to the previous instruction, not the future instruction?
The future instruction didn't start executing yet.
> It is OK to update the function symbol in case a previous event did not provide
> an IP, but I believe we should also handle the case where we do switch functions.
I don't see how we could do that without an IP. Even if GDB would look forward
in the recording, how could it choose which function to put the second
event in without an IP? It could just as well have been two events ending the
first function.
> > static int
> >@@ -1236,6 +1280,7 @@ handle_pt_insn_events (struct btrace_thread_info
> >*btinfo,
> > {
> > struct pt_event event;
> > uint64_t offset;
> >+ CORE_ADDR pc = 0;
>
> Wasn't there some feedback on the ptwrite series to go with local declarations?
> IIRC Simon asked for that.
I guess you are talking about this discussion?
https://sourceware.org/pipermail/gdb-patches/2024-August/211183.html
I think it was mostly about the unnecessary passing of bfun.
I just felt like having a separate declaration for all of these cases is making
the code a bit bloated (11 times the same line instead of 1).
And we already have offset as an example of a variable that is not local
and not used by all cases. Now that I read the code again, we could even
remove offset and have the two cases that use it use pc instead.
Then every single cases would use the variable and we would only use
one instead of two. What do you think?
> >@@ -1251,8 +1296,16 @@ handle_pt_insn_events (struct btrace_thread_info
> >*btinfo,
> > if (event.status_update != 0)
> > break;
> >
> >+ /* Only create a new gap if the last function segment contains
> >+ non-aux instructions. We could be at the beginning of the
> >+ recording and could already have handled one or more events,
> >+ like ptev_iret, that created aux insns. In that case we don't
> >+ want to create a gap or print a warning. Note that if the last
> >+ function segment contains only aux insns, we are guaranteed
> >+ to be at the beginning of a recording or after a gap. See
> >+ handle_pt_aux_insn (). */
>
> If we allow switching functions based on event IPs, the last part of the comment
> no longer holds. It may no longer be sufficient to look at the last function
> segment.
Note that we are in the ptev_enabled case here, and the comment is only
talking about that case. It isn't a generic statement for all events.
Please elaborate if you still see a problem here.
> >+/* This testcase is part of GDB, the GNU debugger.
> >+
> >+ Copyright 2024 Free Software Foundation, Inc.
> >+
> >+ Contributed by Intel Corp. <markus.t.metzger@intel.com>
>
> That part (the email address) isn't quire correct;-) Nor necessary.
Right. Left over from copying. Fixed.
> >+# Test event tracing with gaps.
> >+
> >+require allow_btrace_tests allow_btrace_event_trace_tests
>
> Doesn't the latter imply the former?
>
> Also, we're using 'record btrace pt' below, so allow_btrace_tests would
> not be sufficient, anyway.
I added a "require allow_btrace_pt_tests" to " allow_btrace_event_trace_tests"
and removed the "allow_btrace_tests" from here.
> >+gdb_test_no_output "set record btrace pt event-tracing on"
> >+gdb_test_no_output "record btrace pt"
> >+
> >+set bp_1 [gdb_get_line_number "bp1"]
> >+set bp_2 [gdb_get_line_number "bp2"]
> >+gdb_breakpoint $bp_1
> >+gdb_breakpoint $bp_2
> >+
> >+gdb_test "next"
> >+
> >+gdb_test "call square (3)" [multi_line \
>
> The comment below explains that we do an inferior call to create a gap in the
> trace. It may be good to explain this here where we do the call.
I will move that part of the comment up here.
> >+gdb_test_sequence "record function-call-history" "function-call-history" {
> >+ "\[0-9\]+\tmain"
> >+ "\\\[interrupt: vector = 0x1 \\\(#db\\\)(, ip = 0x\[0-9a-fA-F\]+)?\\\]"
>
> Can we use $hex and $decimal?
I don't know why, but for some reason it doesn't work.
As soon as I change e.g. this:
- "\[0-9\]+\tmain"
+ "$decimal\tmain"
It fails. Same if I only change the hex values.
I use the same regex for a gdb_test with multiline in this very patch, and
there it works. I blame test_sequence for now.
> >+# Test printing of at least one INTERRUPT event.
> >+gdb_test "record function-call-history" [multi_line \
> >+ "$decimal\tmain" \
> >+ "\t \\\[interrupt: vector = 0x1 \\\(#db\\\), ip = $hex\\\]" \
>
> The #db comes from stepping over the breakpoint at main, correct?
Right.
> It is not really part of the test and if we ever want to change runto_main
> to use a temporary breakpoint, we'd need to adjust the test.
We already rely on this to not be a temporary breakpoint when we test
IRET. I get from where you are coming from, but I think this would also be
totally fine to change once someone changes runto_main. The test will fail.
If you think that is not acceptable, we could make this more robust by
setting an explicit breakpoint e.g. in call1, and rely on that.
Though that makes the output more complicated and the test longer.
> >+ "$decimal\tcall1" \
>
> Also, it is not clear to me that we are guaranteed to not have another
> entry in main between stepping over the breakpoint and calling call1.
You are right. I could make this a test sequence, or add some ".*"
I have opted for that latter for now. Neither is perfect.
> >+ "$decimal\tmain" \
> >+ "\t \\\[interrupt: vector = 0xe \\\(#pf\\\), cr2 = 0x0, ip = $hex\\\]"
> >+ ]
>
> Both CR2 and IP are optional in the libipt interface.
I had them as optional in the regex at some point, but you asked me to
remove it in an internal discussion.
You said "We know that there is cr2 for '#pf'." and " There is no cr2 for '#db'"
And you mentioned that you preferred it more readable.
I am fine with it either way.
> >+# Test the instruction-history. Assembly can differ between compilers, to
> >+# avoid creating a .S file for this test we just check the last two lines.
>
> I believe there should be a dot between compilers and to avoid.
Right, fixed locally.
> >+int
> >+main ()
> >+{
> >+ int a = call1 (34);
> >+ int *b = &a;
> >+ b = NULL;
>
> Do we need to initialize b to &a first?
We don't, I will assign NULL directly.
> >+# Run a test on the target to see if GDB supports event tracing on it.
> >+# Return 1 if so, 0 if it does not.
> >+
> >+gdb_caching_proc allow_btrace_event_trace_tests {} {
>
> This check uses 'record pt' so it should probably be called
> allow_btrace_pt_event_trace_tests.
Fixed locally. Though the ptwrite function also doesn't do that.
> >+ global srcdir subdir gdb_prompt inferior_exited_re decimal
>
> The function is not using the last three.
>
Fixed locally.
Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
More information about the Gdb-patches
mailing list