This is an archive of the discontinued LLVM Phabricator instance.

Replace stream-based error handling in the expression parser
ClosedPublic

Authored by spyffe on Mar 14 2016, 4:32 PM.

Details

Summary

We want to do a better job presenting errors that occur when evaluating expressions. Key to this effort is getting away from a model where all errors are spat out onto a stream where the client has to take or leave all of them.

To this end, this patch adds a new class, DiagnosticManager, which contains errors produced by the compiler or by LLDB as an expression is created. The DiagnosticManager can dump itself to a log as well as to a string. Clients will (in the future) be able to filter out the errors they're interested in by ID or present subsets of these errors to the user.

This patch is not intended to change the *users* of errors – only to thread DiagnosticManagers to all the places where streams are used. I also attempt to standardize our use of errors a bit, removing trailing newlines. Going forward, I would like to make clients omit 'error:', 'warning:' etc. and exclusively pass the Severity flag.

The patch is testsuite-neutral, with modifications to one part of the MI tests because it relied on "error: error:" being erroneously printed. This patch fixes that testcase.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe updated this revision to Diff 50676.Mar 14 2016, 4:32 PM
spyffe retitled this revision from to Replace stream-based error handling in the expression parser.
spyffe updated this object.
spyffe added reviewers: jingham, clayborg, ldrumm.
spyffe set the repository for this revision to rL LLVM.
jingham edited edge metadata.Mar 14 2016, 5:27 PM

It worries me that the Diagnostic item has a "compiler specific ID" but so far as I can see there's nothing to tell me what compiler produced the ID. That means the agent consuming the diagnostic has to know a priori what compiler it is getting the diagnostic for or it won't be able to interpret it. That seems like an unnecessary restriction.

If the diagnostic manager's diagnostics were guaranteed to all come from the same compiler, you could record this in the diagnostic. Otherwise it should go in the Diagnostic itself.

But this is a great improvement, and other than that quibble, the code seems good.

clayborg requested changes to this revision.Mar 15 2016, 10:54 AM
clayborg edited edge metadata.

Looks good with a few minor changes as noted in inlined comments.

One question: do we want to be able to ask a DiagnosticManager object how many errors there were? Otherwise everyone has to manually iterate over them to figure out if we got any errors?

include/lldb/Expression/DiagnosticManager.h
23–28

Should we include some flag along with DiagnosticSeverity that says "please file a bug on the compiler, linker, or debugger"?

35

uint64_t for the compiler_id just to be safe and forward looking?

39

This should be inside the DiagnosticManager class as private or protected if it is the only thing that uses it. I am guessing this is only for the DiagnosticManager class?

64–66

It is probably ok to have the compiler_id be default, but "severity" shouldn't be. See comment below in the ClangDiagnosticAdaptor for more insight..

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
178

We should specify eDiagnosticSeverityError here to be clear. I believe the severity should not be a default argument to keep the code easier to read.

This revision now requires changes to proceed.Mar 15 2016, 10:54 AM

Many of Greg's concerns reflect the fact that we're not giving people access to the raw Diagnostic objects and instead optimistically designing API. Upon reflection, I would rather give people access to a const DiagnosticList & and only add mutators when code actually needs them. This, and responses to Jim's and Greg's comments, will be in the next iteration of this patch.

include/lldb/Expression/DiagnosticManager.h
35

I would not do this, simply because I doubt that there will be 4 billion unique diagnostics and because there is no need for binary compatibility of any sort here.

As Jim suggested, though, an origin field would be useful.

39

There are functions that take and return these, namely GetAllDiagnosticsAfter and AppendDiagnostics.

64–66

Good point. I will fix this.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
178

You're right. I will fix this.

ldrumm edited edge metadata.Mar 15 2016, 1:11 PM

Overall this patch is great cleanup!

However, see my comments inline. Most are simply minor implementation details.

Also, the new code introduces some trailing whitespace, and has is inconsistent between void prototype();, void prototype (); and callfunc();, callfunc (); so it may be worth running clang-format-diff` on the changes.

include/lldb/Expression/DiagnosticManager.h
84

I think it might be helpful to have a comment describing the format of the returned string. The implementation joins them with newlines, but this is non-obvious.

source/Expression/DiagnosticManager.cpp
51

Would it be better to batch these, and then dump a single string to the log in one go - akin to what Greg suggested in this diff?
In fact, as you already have the GetString method, you could maybe do:

void
DiagnosticManager::Dump(Log *log)
{
    if (!log)
        return;
    log->PutCString(GetString().c_str());
}

Although the newlines will be different in the current implementation.

You could therefore change the protoype for GetString

cpp
std::string
DiagnosticManager::GetString(char separator='\n')
{
std::string
DiagnosticManager::GetString(char separator)
{
    std::string ret;
    
    for (Diagnostic &diagnostic : m_diagnostics)
    {
        ret.append(diagnostic.message);
        ret.push_back(separator);
    }
    
    return ret;
}
source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
491

num_errors looks to be unsigned I believe %d should be %u

source/Target/Process.cpp
5481

num_resumes is uint32_t : %d -> PRIu32

5498

num_resumes is uint32_t : %d -> PRIu32

ldrumm requested changes to this revision.Mar 15 2016, 1:16 PM
ldrumm edited edge metadata.

Thanks for the comments, Luke! I have responded to your changes, will run clang-format, and will then update this diff.

include/lldb/Expression/DiagnosticManager.h
84

Very true. Will do.

source/Expression/DiagnosticManager.cpp
51

Will do. Thanks. The only difference with the log is the trailing newline. Maybe I'll just manually remove that from the output of GetString.

source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
491

Will fix, thanks.

source/Target/Process.cpp
5481

Will fix.

5498

Will fix.

spyffe updated this revision to Diff 50770.Mar 15 2016, 2:40 PM
spyffe edited edge metadata.

Applied previous comments as discussed before.
Ran ./llvm/tools/clang/tools/clang-format/clang-format-diff.py -b ./llvm-build/Debug+Asserts/x86_64/bin/clang-format -i

clayborg requested changes to this revision.Mar 16 2016, 10:16 AM
clayborg edited edge metadata.

Just change the "origin" variable over to use DiagnosticOrigin as the type and this is good to go.

include/lldb/Expression/DiagnosticManager.h
43

The type of this should be DiagnosticOrigin, not uint32_t

This revision now requires changes to proceed.Mar 16 2016, 10:16 AM
clayborg accepted this revision.Mar 16 2016, 10:16 AM
clayborg edited edge metadata.

After you change "origin" to be a DiagnosticOrigin this is ready.

Thanks. I will apply your suggestion.

include/lldb/Expression/DiagnosticManager.h
43

Yeah, since we're not trying to maintain binary compatibility, I agree.

jingham accepted this revision.Mar 16 2016, 3:03 PM
jingham edited edge metadata.

LGTM

ldrumm accepted this revision.Mar 17 2016, 9:44 AM
ldrumm edited edge metadata.

I haven't had a chance to try this out (at EuroLLVM) but looks good.

This revision is now accepted and ready to land.Mar 17 2016, 9:44 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Seems to be part of r263859.