This is an archive of the discontinued LLVM Phabricator instance.

NFC. Refactored DIPrinter for better support of new print styles.
ClosedPublic

Authored by aorlov on Mar 19 2021, 5:24 PM.

Details

Summary

This patch introduces a DIPrinter interface to implement by different output style printer implementations. DIPrinterGNU and DIPrinterLLVM implement the GNU and LLVM output style printing respectively. No functional changes.

This refactoring clarifies and simplifies the code, and makes a new output style addition easier.

Diff Detail

Event Timeline

aorlov created this revision.Mar 19 2021, 5:24 PM
aorlov requested review of this revision.Mar 19 2021, 5:24 PM
aorlov updated this revision to Diff 332123.Mar 20 2021, 12:23 PM
jhenderson added inline comments.Mar 22 2021, 2:21 AM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
31

I'd be inclined to omit the DI part of this name. We're in a distinct namespace, and I'm not sure the DI prefix gives us anything.

34

StringRef is designed to be passed by value, as it's already a reference, so is cheap to copy.

46

Maybe OutputStream and ErrorStream? ES in particular is not a common abbreviation, so it's hard to follow the naming.

57

See above re. StringRef

60

I can see why you have this class, but I'm not a fan of the name. Why is this "generic", but the base class isn't, for example? Also, see above re. the DI prefix.

How about PlainPrinterBase or something like that? The subclasses could be LLVMPrinter and GNUPrinter.

62

Perhaps we could reduce the number of changes needed in this main part of the refactoring by moving the address printing in a separate prerequisite patch? Does that make sense?

71

Why is this virtual and protected?

82

I believe you don't need this, since it's in the base class.

95–96

Here and everywhere else, use override wherever you are overriding a base class method.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp
215–216

This looks extremely brittle. Using invalid_argument as an error code is not unusual, and probably doesn't always correspond to the case you think it does.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
78

Why have you changed what this function does? What was wrong with the previous approach, where the main printing was handled by the calling code? I'd think you could just pass in the Printer, and then call Printer.printError in place of the logAllUnhandledErrors function.

94–96

This kind of special-casing shows that this isn't the best design for this code.

159–163

This probably wants to be a completely different function, since an unparseable command is not really an error. How about another function printInvalidCommand? That would help resolve the brittleness I mention above, too.

aorlov updated this revision to Diff 332409.Mar 22 2021, 1:23 PM
aorlov marked 10 inline comments as done.Mar 22 2021, 1:40 PM
aorlov added inline comments.
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
46

I have changed the OS and ES to the requested names. On my taste the code became less readable, as the lines are longer and more line breaks are involved. Personally I’d prefer shorter names, OS is already used name people got used to, and ES is in line.

62

I can propose that small patch. Though it does not seem worth it as it will save just a few lines in this one, wouldn’t make a big difference.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
78

I didn’t change the error() function.

I have implemented a new function which effectively does print-res-or-err and implements the logic from main which used to be a sequence of branches specific to different data types. Since the handling of different data types are similar, we could use the template machinery to highlight the similarity. This produces a smaller cleaner code.

Printing out errors is a trivial one-liner, and does not deserve a separate function. That’s a small part of this new print-res-or-err function.

I have named this print-res-or-err function “print”, since I think it is a better name. print(ResOrErr) looks clean and right to the point. Much better than printResOrErr(ResOrErr).

94–96

You are right. We do not need this special case here.

159–163

since an unparseable command is not really an error

What is so special with the unparseable command besides someone attached some questionable logic to that case in the current implementation? This is one of regular errors, which shall be reported with regards to the specified output style.
It might look a bit different just because it is detected here and not returned by a library code. Does not make it something other than error, though.
Weirdness here is because of the bug-to-bug backward compatibility.

aorlov updated this revision to Diff 332474.Mar 22 2021, 4:47 PM
aorlov marked 2 inline comments as done.
jhenderson added inline comments.Mar 26 2021, 2:19 AM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
34

Similarly, no need for the const anymore. Please delete.

46

Having seen the impact, I regret asking you to do this. If you prefer to use OS and ES, it's fine to change back.

59
62

My thinking is that it is actually a beneficial change, even without the other refactoring. It would be good to minimise noise as much as possible.

117–119

I think it would be a good idea to create a helper PrinterConfig class or similar, which contains the various Print* and Verbose argument. It will make the interface cleaner and easier to extend in the future.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp
34
93–137

If I'm reading this function correctly, I think you can push most of it into the base class, because it's very similar to the LLVM style. If you add a few new functions, something like printFunctionName, printSimpleLocation (which would take a filename at least), and printVerbose, with the printSimplyLocation implemented by the subclasses and the base class implementing the other two. The base class print would then call these functions in sequence.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
159–163

Weirdness here is because of the bug-to-bug backward compatibility.

One person's bug is another person's feature. For example, you might want to print some custom delimiters between symbolized addresses.

In the existing printers, the invalid input is not considered an error, whereas it might be for e.g. JSON. I therefore think you should have two functions: printInvalidCommand and printError. For the JSON implementation, printInvalidCommand would just forward straight to printError, saying something like "invalid input addresses are not accepted with JSON output style" (or possibly stuffing it in a JSON object with some unique tag or something. For the LLVM and GNU implementations, the function would just print and then return without needing to do anything special. It helps simplify the printError logic, which I find to be somewhat complicated for an error handling function, if I'm honest.

aorlov updated this revision to Diff 333621.Mar 26 2021, 1:24 PM
aorlov marked 6 inline comments as done.Mar 26 2021, 1:29 PM
aorlov added inline comments.
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
117–119

Adding PrinterConfig does not seem to make this patch better. It would be the same set of flags but passed in to the PrinterConfig constructor, which in its turn will be passed to a DIPrinter instance a line or two below that.

Here is the code snippet to illustrate:

PrinterConfig PrnCfg(Args.hasArg(OPT_addresses),
                     Opts.PrintFunctions != FunctionNameKind::None,
                     Args.hasArg(OPT_pretty_print),
                     SourceContextLines, Args.hasArg(OPT_verbose));

if (OutputStyle == DIPrinter::OutputStyle::GNU)
  Printer.reset(new GNUPrinter(outs(), errs(), PrnCfg));

+ PrinterConfig definition noise.

Note JSONPrinter does not require PrinterConfig at all.

I do not oppose the idea of having PrinterConfig, but I do oppose the idea of making it a dependency to this patch. If someone feels strong for having such class, it could be added later out of the scope of this patch.

jhenderson added inline comments.Mar 29 2021, 12:41 AM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
117–119

You don't have to have a constructor for the PrinterConfig class. Instead, you do something like the following:

class PrinterConfig {
public:
  bool PrintAddress = false; // Default initializer is optional.
  bool PrintFunctions = false;
  ...
};

void setupConfig(...)
{
  PrinterConfig Config;
  Config.PrintAddress = Args.hasArg(OPT_addresses);
  Config.PrintFunctions = Opts.PrintFunctions != FunctionNameKind::None;
  Config.ES = errs();
  ...
}

The difference is that you no longer have a list of boolean arguments that can easily end up in the wrong order. It also avoids potential for things like this:

LLVMPrinter(outs(), errs(), true, false, true, 0, false);

which I hope you'll agree is not all that legible.

This is similar to how the LLD config is created - some options are default initialised, and then argument parsing populates the remainder.

As this is about making the Printer interface nicer, it is part of this patch.

It's also worth pointing out that this removes the duplication of the big argument list where you construct the LLVM/GNUPrinter instances in the llvm-symbolizer code.

aorlov updated this revision to Diff 333799.Mar 29 2021, 3:07 AM
aorlov marked an inline comment as done.Mar 29 2021, 3:18 AM
aorlov added inline comments.
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
117–119

Note PrinterConfig is PlainPrinterBase specific, not the Printer interface.
So I pass PrinterConfig to LLVM/GNUPrinter constructors. We don't need it for JSONPrinter.
I kept OS and ES as is, because Config.OS is very similar to OutputStream story.

aorlov marked an inline comment as done.Mar 30 2021, 1:58 PM

Just for the record.
The build status is red because of a problem not related to this patch.

aorlov updated this revision to Diff 334397.Mar 31 2021, 3:36 AM
dblaikie added inline comments.Apr 2 2021, 3:48 PM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
34–35

I probably wouldn't bother with a ctor here - uses can use braced init to construct it with a similar amount of syntax.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
328–350

If the DIPrinter represents whether to use GNU or LLVM style printing - I don't think the output style should otherwise be exposed to the code (if it is, it seems to suggest that DIPrinter doesn't sufficiently encapsulate the concept of output styling)

Then that DIPrinter::OutputStyle enum could move into llvm-symbolizer.cpp since it won't need to be known by any other code.

329–331

Probably use std::make_unique here

aorlov updated this revision to Diff 335100.Apr 3 2021, 12:56 PM
aorlov marked 3 inline comments as done.Apr 3 2021, 1:08 PM
aorlov added inline comments.
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
34–35

I have removed any streams from DIPrinter. Now it is a clean interface.
Note we don't need stderr in JSONPrinter.
And any other printer might not use the stdout and/or stderr at all.
But I kept OS and ES in PlainPrinterBase ctor.
I don't see how to gracefully use braced init of PlainPrinterBase.

dblaikie added inline comments.Apr 3 2021, 7:21 PM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
34–35

Sorry, I don't think my suggestion has anything to do with OS and ES.

I mean remove the Request ctor, then later on where there's currently Printer.print(Request(ModuleName, Address), *ResOrErr); it can be replaced with Printer.print({ModuleName, Address}, *ResOrErr);

aorlov updated this revision to Diff 335147.Apr 4 2021, 8:50 AM
aorlov marked an inline comment as done.
aorlov marked an inline comment as done.
dblaikie accepted this revision.Apr 4 2021, 7:12 PM

Looks good to me - one optional rename. I don't have a great idea of an alternative though.

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
71

Might be worth renaming this - I'm not sure "Plain" is obviously "not JSON or YAML or whatever". Human readable? Something else?

This revision is now accepted and ready to land.Apr 4 2021, 7:12 PM

Thanks for wrapping up this review @dblaikie. Apologies for disappearing @aorlov. I was already pretty buried with work, and then I've just had two weeks off work, so am only now beginning to catch up on reviews.