[PATCH] D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D89845#2455700 <https://reviews.llvm.org/D89845#2455700>, @clayborg wrote:

> In D89845#2454273 <https://reviews.llvm.org/D89845#2454273>, @jhenderson wrote:
>
>> Looks fine aside from the two outstanding comments re. error messages.
>
> I commented that these are in dump functions that were already created before this patch. They are in the process of dumping the contents to the stream, so dumping the error messages to the stream seems most important. We don't want to stop dumping the information that people asked for due to running into a single error in a file as people will probably want see all of the errors in a file.

Continuing rather than stopping makes sense to me for dumpers. This is the approach we already follow in llvm-readelf and in some other parts of llvm-dwarfdump. In the debug line dumper (which happens to effectively run in parallel to the parser), we report format issues via a callback, which by default prints a warning to stderr, but still continue parsing when we can (see the recoverable error handler). This allows library consumers to handle the problem differently, e.g. if they have warnings as errors enabled. If I follow it correctly, your code here prints it into stdout, without textual colouring, and with the "error:" prefix. This would mean that someone redirecting stderr to a file will not see the errors in that file. I don't think that's what a user would expect.

Essentially, I'm suggesting adding an additional parameter to the dump functions e.g. `function_ref<void(Error)> ErrorCallback` which the caller can pass. Somewhere up the stack, this would for llvm-dwarfdump at least be `defaultWarningHandler`. This would allow other clients to do something different, should they choose to. See my LLVMDev 2018 lightning talk on "Error Handling in Libraries" for an example of what I mean.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:433
+    return createStringError(errc::invalid_argument,
+                             "Operand index %" PRIu32 " is not valid.",
+                             OperandIdx);
----------------
Looks like this message hasn't been reformatted with lower-case first letter and no full stop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89845/new/

https://reviews.llvm.org/D89845