It adds tests for valid and some invalid cases. More checks and tests to come. Note that the patch fixes validation of the Length field because the value does not include the field itself.
In a separate patch, might be good to add error handling to stop when the Header length is reached, rather than when the section end is reached.
I'd probably write these ays "x == 0" rather than "!x"
Potentially another error case could be added here (in another patch) in the case where the list is terminated earlier than the Length specified in the header.
Looks like the old code fails if the range list is empty - and maybe the new code deosn't fail in that case? Worth adding a test case? (it actually seems reasonable to me to support it - you could imagine a CU with only type information in it & so no covered addresses, and having an arange entry describing that so the consumer didn't have to go discover it through parsing the CU, etc)
does "toString()" consume the error?
return error instead of crashing the program
return llvm::Error here?
return an error if Set.extract fails? Seems weird to add error handling to only convert it to bool and ignore the error?
Dump the error if Set.extract() fails?
- Update wordings.
- Replace most of the lit tests to check error cases with gtest-based ones.
- Do not silently absorb the error messages.
- Update existing tests and add new ones to reflect the changes.
Yes. It uses handleAllErrors() internally.
This just checks for the precondition. Callers are responsible to call the method with valid arguments. Therefore, if this triggers, it should be considered as an internal error and fixed.
As this is just a private helper method for DWARFDebugAranges::generate(), I opted to print the errors here.
Done, but it pulled a lot of changes. Maybe it worth to move all them into a separate patch.
Might this eventually want to go in the direction of having fallible errors and errors that can continue, like some of @jhenderson 's work? Should we come up with a consistent strategy for this sort of thing as it seems we've developed a few different error handling parsing schemes at this point.
Yeah, we should a) be consistent (though I don't mind specifically what we standardise on) and b) avoid printing such low-level errors in a library, as it makes things hard for clients. See my lightning talk on the subject that was inspired by my debug line error handling work some time ago: https://www.youtube.com/watch?v=YSEY4pg1YB0