Make the breakpoint log channel more useful

[jingham]

Summary

I was trying to figure out a problem with how a client was seeing the breakpoint events, and was impeded by two things:

1. The BreakpointEventData::Dump did nothing
2. In order to see the breakpoint events you had to turn on the event log stream, and that's really noisy.

I fixed (1) straightforwardly by printing the breakpoint ID and event type. More than that is probably overkill

I fixed (2) by adding a backstop in the "event" log channel printing where the EventData for the event can provide a Log channel. Then I made the BreakpointEventData return the breakpoint log (if it's on of course), so you can see breakpoint event logging & break logging but not all the other event logging.

I didn't go through and make the other EventData subclasses implement this method. It's not guaranteed that having the event data come out along with the regular log traffic is helpful rather than noisy. For the breakpoints it is clearly helpful since there aren't that many events sent by the system.

[jingham]

For instance, one of the other EventData types might want the events too, but only when the verbose flag for their relevant log channel is on.

[kastiglione]

lldb/source/Utility/Broadcaster.cpp

211–215

should this log to both channels? I find it slightly unexpected that an event wouldn't be printed to the events channel.

[kastiglione]

lldb/source/Breakpoint/Breakpoint.cpp

1063–1065

For the location event types, should the location IDs be included?

[jingham]

lldb/source/Breakpoint/Breakpoint.cpp

1063–1065

Could be, or the number changed, etc. I didn't need that so I didn't add it.

lldb/source/Utility/Broadcaster.cpp

211–215

If the events log category is on, then you will get a non-null log and log to the events Log. You only get another log if Events is off. And if the event data returns a Log in the lldb channel it won't matter anyway, in that case this is just a fancy way of doing LogIfAnyCategory...

The only slight oddity here is that if an EventData chose to provide a Log not in the lldb channel (e.g. the gdb-remote channel), then if you did:

log enable -f /tmp/remote-log.txt gdb-remote packets

the events would go to the remote-log.txt, but then if you did:

log enable lldb event

the event traces would stop going to that log file.

Since the main point of this change is to be able to log X-type events when the X log channel/category is on, but the Events log is NOT on, I don't think that's a real problem.

[kastiglione]

lldb/source/Utility/Broadcaster.cpp

211–215

thanks for correcting my misunderstanding

[JDevlieghere]

lldb/include/lldb/Breakpoint/Breakpoint.h

84

Why not a llvm::StringRef?

[jingham]

lldb/include/lldb/Breakpoint/Breakpoint.h

84

This is returning a null terminated C-string. What would be gained by passing it out as a StringRef so that clients had to call c_str on it?

lldb/include/lldb/Breakpoint/Breakpoint.h

 /// then we will check the location for whether to actually stop or not. One
 /// subtle point worth observing here is that you don't actually stop at a
 /// Breakpoint, you always stop at one of its locations.  So the "should stop"
 /// tests are done by the location, not by the breakpoint.
 class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
                    public Stoppoint {
 public:
   static ConstString GetEventIdentifier();
+  static const char *
+      BreakpointEventTypeAsCString(lldb::BreakpointEventType type);
 
   /// An enum specifying the match style for breakpoint settings.  At present
   /// only used for function name style breakpoints.
   enum MatchType { Exact, Regexp, Glob };
 
 private:
   enum class OptionNames : uint32_t { Names = 0, Hardware, LastOptionName };
 
   static const char
       *g_option_names[static_cast<uint32_t>(OptionNames::LastOptionName)];
 
   static const char *GetKey(OptionNames enum_value) {
     return g_option_names[static_cast<uint32_t>(enum_value)];
   }
 
 public:
   class BreakpointEventData : public EventData {
   public:
     BreakpointEventData(lldb::BreakpointEventType sub_type,
                         const lldb::BreakpointSP &new_breakpoint_sp);
 
     ~BreakpointEventData() override;
 
     static ConstString GetFlavorString();
     
+    Log *GetLogChannel() override;
+
     ConstString GetFlavor() const override;
 
     lldb::BreakpointEventType GetBreakpointEventType() const;
 
-    lldb::BreakpointSP &GetBreakpoint();
+    lldb::BreakpointSP GetBreakpoint() const;
 
     BreakpointLocationCollection &GetBreakpointLocationCollection() {
       return m_locations;
     }
 
     void Dump(Stream *s) const override;
 
     static lldb::BreakpointEventType

lldb/include/lldb/Utility/Event.h

class EventData {
   friend class Event;
 
 public:
   EventData();
 
   virtual ~EventData();
 
   virtual ConstString GetFlavor() const = 0;
   
+  virtual Log *GetLogChannel() { return nullptr; }
+  
   virtual void Dump(Stream *s) const;
 
 private:
   virtual void DoOnRemoval(Event *event_ptr) {}
 
   EventData(const EventData &) = delete;
   const EventData &operator=(const EventData &) = delete;
 };

lldb/source/Breakpoint/Breakpoint.cpp

void Breakpoint::SendBreakpointChangedEvent(BreakpointEventData *data) {
 
   if (!m_being_created && !IsInternal() &&
       GetTarget().EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
     GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged, data);
   else
     delete data;
 }
 
+const char *Breakpoint::BreakpointEventTypeAsCString(BreakpointEventType type) {
+  switch (type) {
+    case eBreakpointEventTypeInvalidType: return "invalid";
+    case eBreakpointEventTypeAdded: return "breakpoint added";
+    case eBreakpointEventTypeRemoved: return "breakpoint removed";
+    case eBreakpointEventTypeLocationsAdded: return "locations added";
+    case eBreakpointEventTypeLocationsRemoved: return "locations removed";
+    case eBreakpointEventTypeLocationsResolved: return "locations resolved";
+    case eBreakpointEventTypeEnabled: return "breakpoint enabled";
+    case eBreakpointEventTypeDisabled: return "breakpoint disabled";
+    case eBreakpointEventTypeCommandChanged: return "command changed";
+    case eBreakpointEventTypeConditionChanged: return "condition changed";
+    case eBreakpointEventTypeIgnoreChanged: return "ignore count changed";
+    case eBreakpointEventTypeThreadChanged: return "thread changed";
+    case eBreakpointEventTypeAutoContinueChanged: return "autocontinue changed";
+  };
+}
+
+Log *Breakpoint::BreakpointEventData::GetLogChannel() {
+  return GetLog(LLDBLog::Breakpoints);
+}
+
 Breakpoint::BreakpointEventData::BreakpointEventData(
     BreakpointEventType sub_type, const BreakpointSP &new_breakpoint_sp)
     : m_breakpoint_event(sub_type), m_new_breakpoint_sp(new_breakpoint_sp) {}
 
 Breakpoint::BreakpointEventData::~BreakpointEventData() = default;
 
 ConstString Breakpoint::BreakpointEventData::GetFlavorString() {
   static ConstString g_flavor("Breakpoint::BreakpointEventData");
   return g_flavor;
 }
 
 ConstString Breakpoint::BreakpointEventData::GetFlavor() const {
   return BreakpointEventData::GetFlavorString();
 }
 
-BreakpointSP &Breakpoint::BreakpointEventData::GetBreakpoint() {
+BreakpointSP Breakpoint::BreakpointEventData::GetBreakpoint() const {
   return m_new_breakpoint_sp;
 }
 
 BreakpointEventType
 Breakpoint::BreakpointEventData::GetBreakpointEventType() const {
   return m_breakpoint_event;
 }
 
-void Breakpoint::BreakpointEventData::Dump(Stream *s) const {}
+void Breakpoint::BreakpointEventData::Dump(Stream *s) const {
+  if (!s)
+    return;
+  BreakpointEventType event_type = GetBreakpointEventType();
+  break_id_t bkpt_id = GetBreakpoint()->GetID();
+  s->Format("bkpt: {0} type: {1}", bkpt_id,
+      BreakpointEventTypeAsCString(event_type));
+}
 
 const Breakpoint::BreakpointEventData *
 Breakpoint::BreakpointEventData::GetEventDataFromEvent(const Event *event) {
   if (event) {
     const EventData *event_data = event->GetData();
     if (event_data &&
         event_data->GetFlavor() == BreakpointEventData::GetFlavorString())
       return static_cast<const BreakpointEventData *>(event->GetData());

lldb/source/Utility/Broadcaster.cpp

void Broadcaster::BroadcasterImpl::PrivateBroadcastEvent(EventSP &event_sp,
 
   if (!m_hijacking_listeners.empty()) {
     assert(!m_hijacking_masks.empty());
     hijacking_listener_sp = m_hijacking_listeners.back();
     if ((event_type & m_hijacking_masks.back()) == 0)
       hijacking_listener_sp.reset();
   }
 
-  if (Log *log = GetLog(LLDBLog::Events)) {
+  Log *log = GetLog(LLDBLog::Events);
+  if (!log && event_sp->GetData())
+    log = event_sp->GetData()->GetLogChannel();
+
+  if (log) {
     StreamString event_description;
     event_sp->Dump(&event_description);
     LLDB_LOGF(log,
               "%p Broadcaster(\"%s\")::BroadcastEvent (event_sp = {%s}, "
               "unique =%i) hijack = %p",
               static_cast<void *>(this), GetBroadcasterName(),
               event_description.GetData(), unique,
               static_cast<void *>(hijacking_listener_sp.get()));