This is an archive of the discontinued LLVM Phabricator instance.

Change Error::PutToLog to LLDB_LOG_ERROR
ClosedPublic

Authored by zturner on Feb 3 2017, 1:36 PM.

Details

Summary

Instead of having the Error class (which is in Utility) know about logging (which is in Core), it seems more appropriate to put all logging code together, and teach Log about errors rather than teaching Error about logs. This patch does so.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Feb 3 2017, 1:36 PM
labath edited edge metadata.Feb 3 2017, 1:52 PM

I'm not opposed to this patch, if people really want it, but I don't really see the value of this macro.
Why couldn't I write
LLDB_LOG(log, "foo: {0}", error);
instead of
LLDB_LOG_ERROR(log, error, "foo");
Am I missing something?

labath added a comment.Feb 3 2017, 2:01 PM

As for the modules part, I assumed the Log class will end up in the lowest layer also. I intended to move it there after I am done with it, but we can do it sooner if that is stopping you from doing anything. ( I do agree that it makes sense to reverse the dependency though).

clayborg edited edge metadata.Feb 3 2017, 2:04 PM

I realize the functionality would add a "error: " prefix if it wasn't there, but it seems like we could add this as a formatting option of the lldb_private::Error class? Then we can just switch the logging code to put the error in as a log item and add the extra flag to ensure we print "error:" it is isn't already prefixed with that?

So I agree with Pavel. Let us know what you think

I'm not opposed to this patch, if people really want it, but I don't really see the value of this macro.
Why couldn't I write
LLDB_LOG(log, "foo: {0}", error);
instead of
LLDB_LOG_ERROR(log, error, "foo");
Am I missing something?

I'm all for making it simpler. The reason I did it this way is that I wanted to keep the exact same syntax as before to minimize the impact of the change. It's rather confusing, but suppose you have these two errors:

  1. E1 = Success, message = "The operation completed successfully", code = 0
  2. E2 = Error, message = "The operation failed", code = 0x12345

When you call PutToLog(E1, "While reading the %d'th item", 7) it will log the string While reading the 7'th item err = 0x0
When you call PutToLog(E2, "While reading the %d'th item", 7) it will log the string error: While reading the 7'th item err = The operation failed (0x12345)

If we do what you suggest, then we are relying on the lldb::Error format provider, which does not have the ability to accept this kind of "contextual reason" string (i.e. the string "While reading the %d'th item" in the above example). So if we did this:

LLDB_LOG(log, error, "While reading the {0}'th item", 7)

Then the entire error must be formatted independently of the entire context string. So we could produce the output error: The operation failed (0x12345), context: While reading the 7'th item but this is slightly different from what was being printed before.

I didn't actually try this approach and see if anything broke, because Windows runs much fewer tests than the rest of LLDB, so even if my test suite passed, it wouldn't guarantee that another test somewhere else wasn't relying on the message being exactly as it currently prints.

labath added a comment.Feb 3 2017, 2:20 PM

If we do what you suggest, then we are relying on the lldb::Error format provider, which does not have the ability to accept this kind of "contextual reason" string (i.e. the string "While reading the %d'th item" in the above example). So if we did this:

LLDB_LOG(log, error, "While reading the {0}'th item", 7)

Then the entire error must be formatted independently of the entire context string. So we could produce the output error: The operation failed (0x12345), context: While reading the 7'th item but this is slightly different from what was being printed before.

I didn't actually try this approach and see if anything broke, because Windows runs much fewer tests than the rest of LLDB, so even if my test suite passed, it wouldn't guarantee that another test somewhere else wasn't relying on the message being exactly as it currently prints.

I doubt anything is depending on the exact string printed to the log. We should just make the new output makes sense.
I'd format this message like this:
LLDB_LOG(log, "Reading the {0}'th item: {1}", 7, error)
which should give you something like:
Reading 7th item: Too many open files.
if you want, we can add the "error:" prefix, numeric error code, or something to the format provider,, but this would be enough for me.

Fair enough, I can do that.

zturner updated this revision to Diff 87036.Feb 3 2017, 2:54 PM
labath accepted this revision.Feb 3 2017, 3:03 PM

looks good, just make sure it compiles.

lldb/include/lldb/Core/Log.h
18 ↗(On Diff #87036)

This is also unnecessary.

lldb/include/lldb/Utility/Error.h
14 ↗(On Diff #87036)

All these includes are now unnecessary :)

lldb/source/Host/common/Host.cpp
911 ↗(On Diff #87036)

lol :)

952 ↗(On Diff #87036)

looks like you forgot this one.

This revision is now accepted and ready to land.Feb 3 2017, 3:03 PM

Much better, just clean up the unnecessary includes and we are good to go.

This revision was automatically updated to reflect the committed changes.