diff_order_for_section_name warning instead of crash - #407
HaydnTrigg wants to merge 1 commit into
Conversation
|
Is there any scenario where a left symbol will be paired to a right symbol but that right symbol will not be paired to a left symbol? That doesn't sound correct to me and I believe it would cause a number of subtle issues in other parts of objdiff which assume that symbols are paired both ways, which is why I intentionally made it crash here instead of ignoring the bug and incorrectly showing no diff. A crash is more likely to be reported as a bug than a warning. Actually, where does objdiff even log to, anyway? I've only seen its warnings in the console when running it in debug mode, which most users won't see at all and won't be able to report. |
|
In non-debug builds you won't see the GUI's warning messages because there's no console window by default, but they do show up when running the CLI. I agree this points to a real problem that should be fixed. But what I'm arguing for is that this particular error should go back to being non-fatal, so the tool can keep going instead of crashing which is the behavior it used to do. In my case the warnings come from invalid symbol mappings. I didn't even know the project had bad mappings until I hit this crash. But refusing to generate a report at all, because the default behavior is to crash, seems like the wrong call for this situation. The broken mappings weren't contributing anything to the report before, so skipping past them produces exactly the same output for those symbols. The difference is that now we get a useful warning pointing at the problem. |
|
I agree that objdiff shouldn't crash when the user gives it invalid symbol mappings, but I wasn't aware that it did so until now. Before turning this error into warning, I think it would make more sense to first at least look into the underlying issue causing it, and if it's not simple to fix that then it might be worth considering just making it a warning. Can you open an issue explaining how to reproduce this situation with invalid symbol mappings crashing? I just tried manually editing |
LagoLunatic
left a comment
LagoLunatic
left a comment
There was a problem hiding this comment.
I believe I fixed the root cause of the bug in PR #412 and objdiff now shouldn't pair up bad symbols automatically anymore. Did it fix your crashing object as well?
I think after that bugfix it should only be possible to hit this due to user error, so making this a warning LGTM now
2 participants
Commit f2f813e added in a function diff_order_for_section_name which crashes out with the error message
Failed to find right side symbol for paired left side symbolThis change makes this function log a warning instead of crashing out and also provides context for the expected symbol name and section that is expected.
v3.8.1:
v3.7.1:
Patched:
