Improve error handling in dwarfdump #234
Conversation
examples/dwarfdump.rs
Outdated
| print!("{:indent$}{:27} ", "", attr.name(), indent = indent + 18); | ||
| if flags.raw { | ||
| println!("{:?}", attr.raw_value()); | ||
| } else { | ||
| dump_attr_value(&attr, &unit, debug_loc, debug_ranges, debug_str); | ||
| dump_attr_value(&attr, &unit, debug_loc, debug_ranges, debug_str)?; |
There was a problem hiding this comment.
It would be nice if we printed the error and continued dumping in cases where the error doesn't prevent it, such as here.
There was a problem hiding this comment.
How can I tell which errors should continue dumping and which ones shouldn't?
There was a problem hiding this comment.
We can continue dumping if the iterator doesn't need the result of the operation to find the next item. That's going to need a bit of knowledge of the internals to figure out though. The main other spot that I think is useful are the loops for debug_info.units() and debug_types.units(). There's possibly a couple more similar to these (anything that uses parse_initial_length internally) but it's not worth spending a lot of time on.
There was a problem hiding this comment.
OK, thanks. I'll take a look at that tomorrow if I have time.
examples/dwarfdump.rs
Outdated
| @@ -896,22 +954,22 @@ fn dump_pubnames<R: Reader>(debug_pubnames: &gimli::DebugPubNames<R>, | |||
| cu_offset.0, | |||
| pubname.name().to_string_lossy().unwrap()) | |||
There was a problem hiding this comment.
The function is already returning a Result, so I would remove the unwrap here too, even though it can't fail for EndianBuf. It'll get optimized away, and it makes it easier when looking for other places that unwrap. There might be a few more unwraps like this that you could remove too.
examples/dwarfdump.rs
Outdated
| }; | ||
| let file = header.file(file).expect("File index should be valid"); | ||
| let file = header.file(file).ok_or(Error::MissingFileIndex)?; |
There was a problem hiding this comment.
We could print an error here instead of returning it.
examples/dwarfdump.rs
Outdated
| @@ -633,7 +633,7 @@ fn dump_exprloc<R: Reader>(data: &gimli::Expression<R>, unit: &Unit<R>) -> Resul | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn dump_op<R: Reader>(dwop: gimli::DwOp, op: gimli::Operation<R, R::Offset>, newpc: &R) { | |||
| fn dump_op<R: Reader>(dwop: gimli::DwOp, op: gimli::Operation<R, R::Offset>, newpc: &R) -> Result<()> { | |||
There was a problem hiding this comment.
This function needs Ok(()) at the end now.
There was a problem hiding this comment.
Well, that's embarassing. Should be fixed now.
|
Thanks for responding quickly. It was a pleasure working with you. |
I've changed all of the expect() calls, but I left the unwraps since it doesn't look like there's any way they could actually fail in practice.
I wasn't entirely sure what approach to take here since issue #231 is pretty low on details, so I hope this is OK. If not, let me know what you're looking for and I'll try to fix it up.