This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Move error printing from DWARF classes to Support library. NFC.
ClosedPublic

Authored by vleschuk on Aug 21 2018, 5:23 AM.

Details

Summary

Both DWARFDebugLine and DWARFDebugAddr use the same callback mechanism for handling recoverable errors. They both implemented similar warn() function to be used as such callbacks.

In this revision we introduce Display[Error|Warning|Note]() functions in Support library and get rid of code duplication in DWARF classes.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk created this revision.Aug 21 2018, 5:23 AM
vleschuk added inline comments.Aug 21 2018, 5:25 AM
include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
252 ↗(On Diff #161697)

I know that this exceeds 80 chars per line, but that's what clang-format suggested.

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
252 ↗(On Diff #161697)

clang-format?

include/llvm/Support/Error.h
912 ↗(On Diff #161697)

Comments should have trailing full stops.

I don't understand the phrase here. Better would be to say something like "Print an Error as an error message on stderr". I think the use of WithColor should be irrelevant to the function description.

913 ↗(On Diff #161697)

These should use lower case first for the first letter (i.e. displayError etc).

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
22–23 ↗(On Diff #161697)

Unrelated whitespace change should not be in this commit.

I'm not convinced "Display" is the right term for the message.

This should say "for the function name"

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
252 ↗(On Diff #161697)

Sorry, you posted this whilst I was in the middle of writing the review!

Are you sure about this? It seems unusual, especially as there are ways to deal with it (indent the arguments less).

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

Let's use dumpError().

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
252 ↗(On Diff #161697)

Yes, 100% sure. I specifically checked.

vleschuk added inline comments.Aug 21 2018, 5:50 AM
include/llvm/Support/Error.h
912 ↗(On Diff #161697)

Will do.

913 ↗(On Diff #161697)

Yeah, let's use dumpError().

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
22–23 ↗(On Diff #161697)

Will revert.

vleschuk updated this revision to Diff 161707.Aug 21 2018, 6:09 AM
  • Changed names of functions from Display* to dump*
  • Changed comments
  • Got rid of unrelated whitespace change
  • Manually fixed indentation when clang-format suggested smth strange
vleschuk marked 8 inline comments as done.Aug 21 2018, 6:10 AM
vleschuk added inline comments.Aug 21 2018, 8:37 AM
lib/Support/Error.cpp
127 ↗(On Diff #161707)

Actually it appears that this code is invalid:

WithColor::error() and friends can be used only in-place. If you pass it as param:

void foo(raw_ostream& OS) { OS << "test\n"; }
void bar() { foo(WithColor::error()); }

Will result in two lines:

error: test
error:

What can we do here? The easiest way is duplicate the call to handleAllErrors() in each of 3 functions. But I don't like it.

vleschuk added inline comments.Aug 21 2018, 8:49 AM
lib/Support/Error.cpp
127 ↗(On Diff #161707)

Actually the code looks correct to me, but the actual behavior looks strange.

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

Given that, I think the right thing to do is to just move the DWARFDebugLine::warn function out into the DWARFContext::dump function, and pass it around as necessary (or make it a static function in the file). If you go that approach, I'd be tempted to do the same with an error function too (note that there are several places within DWARFContext.cpp that use WithColor::error()).

Your thoughts?

vleschuk added a comment.EditedAug 22 2018, 5:06 AM

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

Given that, I think the right thing to do is to just move the DWARFDebugLine::warn function out into the DWARFContext::dump function, and pass it around as necessary (or make it a static function in the file).

I like that idea, however, we also have dsymutil which was using DWARFDebugLine's implementation. I think it would be correct to leave warning as function in DWARFContext.h, not as static function.

If you go that approach, I'd be tempted to do the same with an error function too (note that there are several places within DWARFContext.cpp that use WithColor::error()).

While warnings are used in one way only and their purpose is mainly to handleAllErrors, error reporting format is not that unified...

vleschuk updated this revision to Diff 162010.Aug 22 2018, 11:47 AM
  • Removed all functions from Support library in order not to tempt other Support users
  • dumpWarning() now lives in DWARFContext
  • Had to get rid of default callbacks in DebugLine and DebugAddr to avoid circular dependency with DWARFContext

@jhenderson Here is the variant where dumpWarning() lives in DWARFContext. Patch became much simpler and smaller. What do you think?

jhenderson accepted this revision.Aug 23 2018, 5:22 AM

Okay, LGTM.

This revision is now accepted and ready to land.Aug 23 2018, 5:22 AM
This revision was automatically updated to reflect the committed changes.