wallace created this revision.
wallace added reviewers: jj10306, persona0220.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The low-level decoder might fall into an infinite decoding loop for
various reasons, the simplest being an infinite direct loop reached due
to wrong handling of self-modified in the kernel, e.g.

  0x0A: pause
  0x0C: jump to 0x0A

In this case, all the code is sequential and requires no packets to be
decoded. The low-level decoder would produce an output like the
following

  0x0A: pause
  0x0C: jump to 0x0A
  0x0A: pause
  0x0C: jump to 0x0A
  0x0A: pause
  0x0C: jump to 0x0A
  ... infinite amount of times

These cases are pretty much undecodable, so the least we can do is to
identify if we have found a case like this one and show then an error in
the trace.

- Add a check that breaks decoding of a single PSB once 200k

instructions have been decoding after the last packet was processed.

- Add a `settings` property for tweaking this number. This is nice

because does the basic work needed for future settings.

Some notes are added in the code. I haven't been unable to create a test
case, but it's found in the thread #12 of the trace
72533820-3eb8-4465-b8e4-4e6bf0ccca99 at Meta. We have to figure out how to
artificially create traces with this kind of anomalies.

With this change, that anomalous thread now shows:

  (lldb) thread trace dump instructions 12 -e -i 213100
  
  thread #12: tid = 8
      ...missing instructions
      213100: (error) decoding truncated: possible infinite decoding loop 
detected
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 7 at 
panic.c:87:2
      213099: 0xffffffff81342787    jmp    0xffffffff81342785        ; <+5> 
[inlined] rep_nop at processor.h:13:2
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 5 
[inlined] rep_nop at processor.h:13:2
      213098: 0xffffffff81342785    pause
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 7 at 
panic.c:87:2
      213097: 0xffffffff81342787    jmp    0xffffffff81342785        ; <+5> 
[inlined] rep_nop at processor.h:13:2
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 5 
[inlined] rep_nop at processor.h:13:2
      213096: 0xffffffff81342785    pause
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 7 at 
panic.c:87:2
      213095: 0xffffffff81342787    jmp    0xffffffff81342785        ; <+5> 
[inlined] rep_nop at processor.h:13:2
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 5 
[inlined] rep_nop at processor.h:13:2
      213094: 0xffffffff81342785    pause
    vmlinux-5.12.0-0_fbk8_clang_6656_gc85768aa64da`panic_smp_self_stop + 7 at 
panic.c:87:2
      213093: 0xffffffff81342787    jmp    0xffffffff81342785        ; <+5> 
[inlined] rep_nop at processor.h:13:2
    ...

It used to be in an infinite loop.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136557

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTProperties.td

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTProperties.td
===================================================================
--- /dev/null
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTProperties.td
@@ -0,0 +1,13 @@
+include "../../../../include/lldb/Core/PropertiesBase.td"
+
+let Definition = "traceintelpt" in {
+  def InfiniteDecodingLoopVerificationThreshold:
+      Property<"infinite-decoding-loop-veritication-threshold", "UInt64">,
+    Global,
+    DefaultUnsignedValue<200000>,
+    Desc<"Specify how many instructions following an individual Intel PT "
+      "packet must have been decoded before triggering the verification for "
+      "infinite decoding loops, which might happen if the image is corrupted, "
+      "such as with self-modifying code. If an infinite loop is detected, an "
+      "error will be included inside the trace.">;
+}
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -22,6 +22,21 @@
 
 class TraceIntelPT : public Trace {
 public:
+  /// Properties to be used with the `settings` command.
+  class PluginProperties : public Properties {
+  public:
+    static ConstString GetSettingName();
+
+    PluginProperties();
+
+    ~PluginProperties() override = default;
+
+    uint64_t GetInfiniteDecodingLoopVerificationThreshold();
+  };
+
+  /// Return the global properties for this trace plug-in.
+  static PluginProperties &GetGlobalProperties();
+
   void Dump(Stream *s) const override;
 
   llvm::Expected<FileSpec> SaveToDisk(FileSpec directory,
@@ -59,6 +74,8 @@
   CreateInstanceForLiveProcess(Process &process);
 
   static llvm::StringRef GetPluginNameStatic() { return "intel-pt"; }
+
+  static void DebuggerInitialize(Debugger &debugger);
   /// \}
 
   lldb::CommandObjectSP
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -16,6 +16,7 @@
 #include "TraceIntelPTBundleSaver.h"
 #include "TraceIntelPTConstants.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "llvm/ADT/None.h"
@@ -39,11 +40,51 @@
       new CommandObjectThreadTraceStartIntelPT(*this, interpreter));
 }
 
+#define LLDB_PROPERTIES_traceintelpt
+#include "TraceIntelPTProperties.inc"
+
+enum {
+#define LLDB_PROPERTIES_traceintelpt
+#include "TraceIntelPTPropertiesEnum.inc"
+};
+
+ConstString TraceIntelPT::PluginProperties::GetSettingName() {
+  return ConstString(TraceIntelPT::GetPluginNameStatic());
+}
+
+TraceIntelPT::PluginProperties::PluginProperties() : Properties() {
+  m_collection_sp = std::make_shared<OptionValueProperties>(GetSettingName());
+  m_collection_sp->Initialize(g_traceintelpt_properties);
+}
+
+uint64_t
+TraceIntelPT::PluginProperties::GetInfiniteDecodingLoopVerificationThreshold() {
+  const uint32_t idx = ePropertyInfiniteDecodingLoopVerificationThreshold;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+      nullptr, idx, g_traceintelpt_properties[idx].default_uint_value);
+}
+
+TraceIntelPT::PluginProperties &TraceIntelPT::GetGlobalProperties() {
+  static TraceIntelPT::PluginProperties g_settings;
+  return g_settings;
+}
+
 void TraceIntelPT::Initialize() {
-  PluginManager::RegisterPlugin(GetPluginNameStatic(), "Intel Processor Trace",
-                                CreateInstanceForTraceBundle,
-                                CreateInstanceForLiveProcess,
-                                TraceIntelPTBundleLoader::GetSchema());
+  PluginManager::RegisterPlugin(
+      GetPluginNameStatic(), "Intel Processor Trace",
+      CreateInstanceForTraceBundle, CreateInstanceForLiveProcess,
+      TraceIntelPTBundleLoader::GetSchema(), DebuggerInitialize);
+}
+
+void TraceIntelPT::DebuggerInitialize(Debugger &debugger) {
+  if (!PluginManager::GetSettingForProcessPlugin(
+          debugger, PluginProperties::GetSettingName())) {
+    const bool is_global_setting = true;
+    PluginManager::CreateSettingForTracePlugin(
+        debugger, GetGlobalProperties().GetValueProperties(),
+        ConstString("Properties for the intel-pt trace plug-in."),
+        is_global_setting);
+  }
 }
 
 void TraceIntelPT::Terminate() {
Index: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
@@ -155,9 +155,10 @@
   ///     appended to. It might have already some instructions.
   PSBBlockDecoder(PtInsnDecoderUP &&decoder_up, const PSBBlock &psb_block,
                   Optional<lldb::addr_t> next_block_ip,
-                  DecodedThread &decoded_thread)
+                  DecodedThread &decoded_thread, TraceIntelPT &trace_intel_pt)
       : m_decoder_up(std::move(decoder_up)), m_psb_block(psb_block),
-        m_next_block_ip(next_block_ip), m_decoded_thread(decoded_thread) {}
+        m_next_block_ip(next_block_ip), m_decoded_thread(decoded_thread),
+        m_trace_intel_pt(trace_intel_pt) {}
 
   /// \param[in] trace_intel_pt
   ///     The main Trace object that own the PSB block.
@@ -192,7 +193,7 @@
       return decoder_up.takeError();
 
     return PSBBlockDecoder(std::move(*decoder_up), psb_block, next_block_ip,
-                           decoded_thread);
+                           decoded_thread, trace_intel_pt);
   }
 
   void DecodePSBBlock() {
@@ -213,12 +214,65 @@
   }
 
 private:
-  /// Decode all the instructions and events of the given PSB block.
-  ///
-  /// \param[in] status
-  ///   The status that was result of synchronizing to the most recent PSB.
+  /// Decode all the instructions and events of the given PSB block. The
+  /// decoding loop might stop abruptly if an infinite decoding loop is
+  /// detected.
   void DecodeInstructionsAndEvents(int status) {
     pt_insn insn;
+    size_t insn_added_since_last_packet = 0;
+    lldb::addr_t last_packet_offset = LLDB_INVALID_ADDRESS;
+    const size_t infinite_loop_threshold =
+        m_trace_intel_pt.GetGlobalProperties()
+            .GetInfiniteDecodingLoopVerificationThreshold();
+
+    // Append an instruction and return false if and only if an infinite
+    // decoding loop has been detected.
+    auto append_instruction = [&]() -> bool {
+      m_decoded_thread.AppendInstruction(insn);
+
+      // We want to check if we might have fallen in an infinite loop. We do so
+      // by checking how many instructions we have added since we processed the
+      // last Intel PT packet. This number should be low, because at some point
+      // we should see branches or interrupts that will require a new packet to
+      // be processed.
+      lldb::addr_t new_packet_offset;
+      if (!IsLibiptError(
+              pt_insn_get_offset(m_decoder_up.get(), &new_packet_offset)) &&
+          new_packet_offset != last_packet_offset) {
+        last_packet_offset = new_packet_offset;
+        insn_added_since_last_packet = 0;
+      }
+
+      // We don't expect a single packet to give, say, 100k instructions. That
+      // would mean that there are 100k sequential instructions without any
+      // single branch, which is highly unlikely, or that we found an infinite
+      // loop using direct jumps, e.g.
+      //
+      //   0x0A: nop or pause
+      //   0x0C: jump to 0x0A
+      //
+      // which is indeed code that is found in the kernel. I presume we reach
+      // this kind of code in the decoder because we don't handle self-modified
+      // code.
+      //
+      // As a note for the future, right now we only check for the
+      // number of instructions, but if this fails, we need to really identify
+      // if there's an infinite loop, which is doable in linear time using the
+      // Knuth-Morris-Pratt algorithm looking for any suffix of the trace that
+      // repeats itself.
+      //
+      // Finally, we are right now only signaling an error, but it would be
+      // more conservative to also discard all the trace items found in this
+      // PSB. I prefer not to do that now to give more exposure to this kind
+      // of anomalies.
+      if (++insn_added_since_last_packet >= infinite_loop_threshold) {
+        m_decoded_thread.AppendCustomError(
+            "decoding truncated: possible infinite decoding loop detected");
+        return false;
+      }
+      return true;
+    };
+
     while (true) {
       status = ProcessPTEvents(status);
 
@@ -238,7 +292,9 @@
       } else if (IsEndOfStream(status)) {
         break;
       }
-      m_decoded_thread.AppendInstruction(insn);
+
+      if (!append_instruction())
+        return;
     }
 
     // We need to keep querying non-branching instructions until we hit the
@@ -247,7 +303,8 @@
     // https://github.com/intel/libipt/blob/master/doc/howto_libipt.md#parallel-decode
     if (m_next_block_ip && insn.ip != 0) {
       while (insn.ip != *m_next_block_ip) {
-        m_decoded_thread.AppendInstruction(insn);
+        if (!append_instruction())
+          return;
 
         status = pt_insn_next(m_decoder_up.get(), &insn, sizeof(insn));
 
@@ -313,6 +370,7 @@
   PSBBlock m_psb_block;
   Optional<lldb::addr_t> m_next_block_ip;
   DecodedThread &m_decoded_thread;
+  TraceIntelPT &m_trace_intel_pt;
 };
 
 Error lldb_private::trace_intel_pt::DecodeSingleTraceForThread(
Index: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
+++ lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
@@ -13,6 +13,14 @@
   SOURCE TraceIntelPTOptions.td
   TARGET TraceIntelPTOptionsGen)
 
+lldb_tablegen(TraceIntelPTProperties.inc -gen-lldb-property-defs
+  SOURCE TraceIntelPTProperties.td
+  TARGET TraceIntelPTPropertiesGen)
+
+lldb_tablegen(TraceIntelPTPropertiesEnum.inc -gen-lldb-property-enum-defs
+  SOURCE TraceIntelPTProperties.td
+  TARGET TraceIntelPTPropertiesEnumGen)
+
 add_lldb_library(lldbPluginTraceIntelPT PLUGIN
   CommandObjectTraceStartIntelPT.cpp
   DecodedThread.cpp
@@ -38,4 +46,7 @@
   )
 
 
-add_dependencies(lldbPluginTraceIntelPT TraceIntelPTOptionsGen)
+add_dependencies(lldbPluginTraceIntelPT
+  TraceIntelPTOptionsGen
+  TraceIntelPTPropertiesGen
+  TraceIntelPTPropertiesEnumGen)
Index: lldb/source/Core/PluginManager.cpp
===================================================================
--- lldb/source/Core/PluginManager.cpp
+++ lldb/source/Core/PluginManager.cpp
@@ -1051,9 +1051,10 @@
       llvm::StringRef name, llvm::StringRef description,
       CallbackType create_callback_from_bundle,
       TraceCreateInstanceForLiveProcess create_callback_for_live_process,
-      llvm::StringRef schema)
+      llvm::StringRef schema, DebuggerInitializeCallback debugger_init_callback)
       : PluginInstance<TraceCreateInstanceFromBundle>(
-            name, description, create_callback_from_bundle),
+            name, description, create_callback_from_bundle,
+            debugger_init_callback),
         schema(schema),
         create_callback_for_live_process(create_callback_for_live_process) {}
 
@@ -1072,10 +1073,10 @@
     llvm::StringRef name, llvm::StringRef description,
     TraceCreateInstanceFromBundle create_callback_from_bundle,
     TraceCreateInstanceForLiveProcess create_callback_for_live_process,
-    llvm::StringRef schema) {
+    llvm::StringRef schema, DebuggerInitializeCallback debugger_init_callback) {
   return GetTracePluginInstances().RegisterPlugin(
       name, description, create_callback_from_bundle,
-      create_callback_for_live_process, schema);
+      create_callback_for_live_process, schema, debugger_init_callback);
 }
 
 bool PluginManager::UnregisterPlugin(
@@ -1506,6 +1507,7 @@
 static const char *kDynamicLoaderPluginName("dynamic-loader");
 static const char *kPlatformPluginName("platform");
 static const char *kProcessPluginName("process");
+static const char *kTracePluginName("trace");
 static const char *kObjectFilePluginName("object-file");
 static const char *kSymbolFilePluginName("symbol-file");
 static const char *kJITLoaderPluginName("jit-loader");
@@ -1559,6 +1561,14 @@
                                 properties_sp, description, is_global_property);
 }
 
+bool PluginManager::CreateSettingForTracePlugin(
+    Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
+    ConstString description, bool is_global_property) {
+  return CreateSettingForPlugin(debugger, ConstString(kTracePluginName),
+                                ConstString("Settings for trace plug-ins"),
+                                properties_sp, description, is_global_property);
+}
+
 lldb::OptionValuePropertiesSP
 PluginManager::GetSettingForObjectFilePlugin(Debugger &debugger,
                                              ConstString setting_name) {
Index: lldb/include/lldb/Core/PluginManager.h
===================================================================
--- lldb/include/lldb/Core/PluginManager.h
+++ lldb/include/lldb/Core/PluginManager.h
@@ -342,7 +342,8 @@
       llvm::StringRef name, llvm::StringRef description,
       TraceCreateInstanceFromBundle create_callback_from_bundle,
       TraceCreateInstanceForLiveProcess create_callback_for_live_process,
-      llvm::StringRef schema);
+      llvm::StringRef schema,
+      DebuggerInitializeCallback debugger_init_callback);
 
   static bool
   UnregisterPlugin(TraceCreateInstanceFromBundle create_callback);
@@ -487,6 +488,10 @@
       Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
       ConstString description, bool is_global_property);
 
+  static bool CreateSettingForTracePlugin(
+      Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
+      ConstString description, bool is_global_property);
+
   static lldb::OptionValuePropertiesSP
   GetSettingForObjectFilePlugin(Debugger &debugger, ConstString setting_name);
 
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to