[PATCH v2 4/6] btrace: Add support for interrupt events.
Willgerodt, Felix
felix.willgerodt@intel.com
Mon Sep 23 08:45:04 GMT 2024
> We explicitly add the current PC to the end of the trace in btrace_add_pc(),
> which is called from btrace_finalize_ftrace_pt().
>
> There's a test case for this in gdb.btrace/segv.exp, but it doesn't check the
> function-call-history or it would have noticed that we stopped switching to
> a new bfun.
Ah ok, I wasn't aware of this. Thanks for pointing that out.
>
> >> It might be safer to use a separate flag to denote the absence of an IP
> >> for events. Once we introduced such a flag, we can also use it for ptwrite.
> >
> >I am not sure where exactly you would see me add this. Just a parameter to
> >this function?
>
> Yes.
Ok, I have implemented this.
> >> >+/* To have a function-call-history. */
> >> >+int
> >> >+call1 (int a)
> >> >+{
> >> >+ return a;
> >> >+}
> >>
> >> I don't think we really need that for the test.
> >>
> >> We'll just have main in the function-call-history, but we cover
> >> longer histories in the other test. This test is checking that we
> >> get the #pf event.
> >
> >I disagree, we use this testfile for all *.exp files. This helps
> >to test that events are in the right function.
> >We are not only testing #pf, we also test IRET later.
>
> You can't iret from a #pf. Well, you can, but you'd just get the #pf
> again, unless you jumped. And we're not covering any of that, here.
> Also, breakpoints, as well as their corresponding IRETs, are covered
> in the other test.
I was actually incorrect, we are using two different *.c files. Sorry that
was my fault, I will remove the function.
>
> >Thanks, I think we need to agree on the nullptr story before
> >getting this merged or re-posted.
>
> Let's discuss this here. Once we agree, the changes should be straight-forward
> and wouldn't need a v3.
>
> Thanks,
> Markus.
I updated some comments and added the flag.
Here is my new diff of btrace.c for this patch:
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 649e0ad59d7..85324ff0276 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -544,10 +544,12 @@ ftrace_new_gap (struct btrace_thread_info *btinfo, int errcode,
/* Update the current function segment at the end of the trace in BTINFO with
respect to the instruction at PC. This may create new function segments.
- Return the chronologically latest function segment, never NULL. */
+ Return the chronologically latest function segment, never NULL.
+ If VALID_PC is false, skip analyzing whether the function has changed. */
static struct btrace_function *
-ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
+ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc,
+ const bool valid_pc = true)
{
struct minimal_symbol *mfun;
struct symbol *fun;
@@ -572,6 +574,11 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
if (bfun->errcode != 0)
return ftrace_new_function (btinfo, mfun, fun);
+ /* If there is no valid PC, which can happen for events with a
+ suppressed IP, we can't do more than return the last bfun. */
+ if (!valid_pc)
+ return bfun;
+
/* Check the last instruction, if we have one.
We do this check first, since it allows us to fill in the call stack
links in addition to the normal flow links. */
@@ -642,6 +649,18 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
break;
}
+
+ 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;
+ }
+ break;
}
}
@@ -668,6 +687,8 @@ ftrace_update_insns (struct btrace_function *bfun, const btrace_insn &insn)
if (insn.iclass == BTRACE_INSN_AUX)
bfun->flags |= BFUN_CONTAINS_AUX;
+ else
+ bfun->flags |= BFUN_CONTAINS_NON_AUX;
if (record_debug > 1)
ftrace_debug (bfun, "update insn");
@@ -1211,10 +1232,11 @@ pt_btrace_insn (const struct pt_insn &insn)
static void
handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
- CORE_ADDR ip)
+ CORE_ADDR pc, const bool valid_pc)
{
btinfo->aux_data.emplace_back (std::move (aux_str));
- struct btrace_function *bfun = ftrace_update_function (btinfo, ip);
+ struct btrace_function *bfun = ftrace_update_function (btinfo, pc,
+ valid_pc);
btrace_insn insn {{btinfo->aux_data.size () - 1}, 0,
BTRACE_INSN_AUX, 0};
@@ -1222,8 +1244,47 @@ handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
ftrace_update_insns (bfun, insn);
}
+/* Check if the recording contains real instructions and not only auxiliary
+ instructions since the last gap (or since the beginning). */
+
+static bool
+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info *btinfo)
+{
+ const std::vector<btrace_function> &functions = btinfo->functions;
+
+ std::vector<btrace_function>::const_reverse_iterator rit;
+ for (rit = functions.crbegin (); rit != functions.crend (); ++rit)
+ {
+ if (rit->errcode != 0)
+ return false;
+
+ if ((rit->flags & BFUN_CONTAINS_NON_AUX) != 0)
+ return true;
+ }
+
+ return false;
+}
#endif /* defined (HAVE_PT_INSN_EVENT) */
+#if (LIBIPT_VERSION >= 0x201)
+/* Translate an interrupt vector to a mnemonic string as defined for x86.
+ Returns nullptr if there is none. */
+
+static const char *
+decode_interrupt_vector (const uint8_t vector)
+{
+ static const char *mnemonic[]
+ = { "#de", "#db", nullptr, "#bp", "#of", "#br", "#ud", "#nm",
+ "#df", "#mf", "#ts", "#np", "#ss", "#gp", "#pf", nullptr,
+ "#mf", "#ac", "#mc", "#xm", "#ve", "#cp" };
+
+ if (vector < (sizeof (mnemonic) / sizeof (mnemonic[0])))
+ return mnemonic[vector];
+
+ return nullptr;
+}
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
+
/* Handle instruction decode events (libipt-v2). */
static int
@@ -1236,6 +1297,8 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
{
struct pt_event event;
uint64_t offset;
+ CORE_ADDR pc = 0;
+ bool valid_pc = false;
status = pt_insn_event (decoder, &event, sizeof (event));
if (status < 0)
@@ -1251,8 +1314,13 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
if (event.status_update != 0)
break;
+ /* Only create a new gap if there are non-aux instructions in
+ the trace since the last gap. 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. */
if (event.variant.enabled.resumed == 0
- && !btinfo->functions.empty ())
+ && ftrace_contains_non_aux_since_last_gap (btinfo))
{
struct btrace_function *bfun
= ftrace_new_gap (btinfo, BDE_PT_NON_CONTIGUOUS, gaps);
@@ -1282,7 +1350,6 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
#if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
case ptev_ptwrite:
{
- uint64_t pc = 0;
std::optional<std::string> ptw_string;
/* Lookup the PC if available. The event often doesn't provide
@@ -1316,6 +1383,8 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
if (pc == 0)
warning (_("Failed to determine the PC for ptwrite."));
+ else
+ valid_pc = true;
if (btinfo->ptw_callback_fun != nullptr)
ptw_string
@@ -1328,11 +1397,40 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
if (!ptw_string.has_value ())
*ptw_string = hex_string (event.variant.ptwrite.payload);
- handle_pt_aux_insn (btinfo, *ptw_string, pc);
+ handle_pt_aux_insn (btinfo, *ptw_string, pc, valid_pc);
break;
}
#endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
+
+#if (LIBIPT_VERSION >= 0x201)
+ case ptev_interrupt:
+ {
+ std::string aux_string = std::string (_("interrupt: vector = "))
+ + hex_string (event.variant.interrupt.vector);
+
+ const char *decoded
+ = decode_interrupt_vector (event.variant.interrupt.vector);
+ if (decoded != nullptr)
+ aux_string += std::string (" (") + decoded + ")";
+
+ if (event.variant.interrupt.has_cr2 != 0)
+ {
+ aux_string += std::string (", cr2 = ")
+ + hex_string (event.variant.interrupt.cr2);
+ }
+
+ if (event.ip_suppressed == 0)
+ {
+ pc = event.variant.interrupt.ip;
+ valid_pc = true;
+ aux_string += std::string (", ip = ") + hex_string (pc);
+ }
+
+ handle_pt_aux_insn (btinfo, aux_string, pc, valid_pc);
+ break;
+ }
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
}
}
#endif /* defined (HAVE_PT_INSN_EVENT) */
The subsequent patches have the same mechanical changes applied, setting valid_pc.
And I fixed the rest of the nits you had locally. Is this ok?
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