This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Report debugger diagnostics as events
ClosedPublic

Authored by JDevlieghere on Mar 11 2022, 9:31 PM.

Details

Summary

Report warnings and errors through events instead of printing directly the to the debugger's error stream. Diagnostic events are handled by the default event loop. This allows IDEs such as Xcode to report these issues in the UI instead of having them show up in the debugger console.

Diff Detail

Event Timeline

JDevlieghere created this revision.Mar 11 2022, 9:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 9:31 PM

Use the asynchronous error stream

It seems to me that it would be better to decentralize the warning tracking. Also due to a central list of all possible warnings not being modular, but mainly because I can imagine that some warnings may want to be reported with different "scopes" -- once per target, thread, module (this is the case of the "dwp missing" warning) or whatever. That won't work if the list is managed centrally in the debugger object. We could define some token warn_once type (maybe just a typedef to once_flag or atomic_flag) that would have to be passed to the warning emitting function. The function would handle the actual "once" logic, and the scope of the token variable would determine the frequency of the warning.

My second remark is that this does not help with the warnings that are not tied to a specific debugger instance -- such as those coming from the module parsing code. I don't know if you have a need to display those (I know I don't), but it may be nice to set things up so that we're able to transmit those as well -- I believe we are able to do that for progress events.

lldb/include/lldb/Core/DebuggerEvents.h
91

Address code review:

  • Decentralize tracking whether a warning/error has been emitted.
  • Support diagnostics that are not tied to a single debugger.
JDevlieghere marked an inline comment as done.

Heh, I didn't realize that "debugger-less" events are implemented by iterating over all debugger instances, but since we're doing that for progress events anyway, we might as well copy the pattern.

Any chance of a test case for some of this stuff? Maybe a unittest which creates a Debugger object and uses it to send/receive some events?

lldb/include/lldb/Core/DebuggerEvents.h
73

Do we really need separate types for this? The handling of the two events is completely identical (except for the "error"/"warning" prefix, so it seems that an enum would suffice. The classes won't make it to the SB api (I think), so it's not like we're locking ourselves into a particular design.

lldb/source/Core/Debugger.cpp
1665–1671

StartListeningForEvents(eBroadcastBitProgress | eBroadcastBitWarning | eBroadcastBitError) ?

1866

GetAsyncErrorStream returns a (shared -- it should probably be a unique_ptr instead) pointer to a new object. You can't just throw it away here.

JDevlieghere marked 2 inline comments as done.

Address code review feedback

Print diagnostics directly to the debugger's error stream if nobody is listening for them.

Migrate a few more call sites to Debugger::Report{Error,Warning}

Harbormaster completed remote builds in B154511: Diff 415699.
labath accepted this revision.Mar 16 2022, 7:02 AM

cool

lldb/unittests/Core/DiagnosticEventTest.cpp
54–55

I guess you could move this into the SetUp function as well.

This revision is now accepted and ready to land.Mar 16 2022, 7:02 AM
This revision was landed with ongoing or failed builds.Mar 16 2022, 8:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 8:33 AM