This is an archive of the discontinued LLVM Phabricator instance.

Add new bugreport command to lldb
ClosedPublic

Authored by tberghammer on Jul 1 2015, 5:10 AM.

Details

Summary

Add new bugreport command to lldb

The new command add functionality to print out domain specific
information for reporting a bug. Currently the only supported
domain is stack unwinding (with "bugreport unwind") but adding
new domains is fairly easy.

Diff Detail

Event Timeline

tberghammer updated this revision to Diff 28857.Jul 1 2015, 5:10 AM
tberghammer retitled this revision from to Add new bugreport command to lldb.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added reviewers: clayborg, ovyalov.
tberghammer added a subscriber: Unknown Object (MLST).
labath added a subscriber: labath.Jul 1 2015, 5:44 AM
labath added inline comments.
source/Commands/CommandObjectBugreport.cpp
88

In no-append mode, you will start overwriting the file without first erasing it's contents. This leads to funny-looking files and is a pretty useless mode. I recommend setting eOpenOptionAppend unconditionally and adding eOpenOptionTruncate if --append is *not* specified.

tberghammer updated this revision to Diff 28867.Jul 1 2015, 7:36 AM

Address review comment about append and truncate

jingham added a subscriber: jingham.Jul 1 2015, 9:19 AM

So much of the useful bug reporting information in lldb is gathered by turning on logs BEFORE the bad thing occurs, then making it happen, then gathering the logs that were produced as a part of that process. For instance, once a bad unwind has occurred that info is cached, and you can't watch lldb figure that out any more. And for expressions or stepping, you really can't do much after the fact.

So maybe a transactional design might be better, where you say

(lldb) bugreport begin <AREA>
...
(lldb) do stuff

(lldb) bugreport end <AREA>

or if you want them to nest you might hand back a token, but that seems over-designing.

I agree that we can collect only a very limited information with a single command after the problem already occurred but my intuition is that if we want the user to reproduce the issue then there is much lower chance that they will submit a bug report even for people working on LLDB.

For stack unwinding in special it is significantly easier to find out what was the issue if we have logs but in most of the case the information I display also sufficient (usually the CFI is the part what is incorrect). I think the same is true for expression evaluation with the only different that we should ask the user to specify the expression for the bugreport command (or we can try to find it on the command history).

ovyalov added inline comments.Jul 1 2015, 10:59 AM
source/Commands/CommandObjectBugreport.cpp
73

I'd defined this buffer before the loop to avoid memory allocation per each frame.

89

File::eOpenOptionCloseOnExec ?

95

Could you add a returned error to result.AppendErrorWithFormat ?

107

This block might be generic for all multiword commands and you may put it into CommandObjectMultiwordBugreport

clayborg accepted this revision.Jul 1 2015, 11:35 AM
clayborg edited edge metadata.

Good start. We can iterate on it to improve it. Sounds like for unwinding we can get away with a single command that gets run afterward. For more complex things, we can implement things like Jim was requesting, but they should probably be more domain specific:

(lldb) bug report step start
// Now make problem happen...
(lldb) s
(lldb) bug report step stop
This revision is now accepted and ready to land.Jul 1 2015, 11:35 AM
tberghammer added inline comments.Jul 2 2015, 3:03 AM
source/Commands/CommandObjectBugreport.cpp
73

Done

89

Done (I don't think it matters here but definitely won't harm)

95

Done

107

In command objects "multiword" refers to the multiple word in the command itself, not to the fact it uses several other commands in the background (see CommandObjectMultiwordTarget). At the moment I am not sure how the other bugreporting commands will look like so don't want to over-engineer it with an additional base class. If we need it anywhere else we can do it when adding the next command using the same pattern.

This revision was automatically updated to reflect the committed changes.