[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

This revision was automatically updated to reflect the committed changes. Closed by commit rL370363: DWARFDebugLoc: Make parsing and error reporting more robust (authored by labath, committed by ). Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

JDevlieghere added a comment. LGTM Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath updated this revision to Diff 217378. labath added a comment. Rebase the patch on top of DataExtractor Cursor changes. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 Files:

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

clayborg added a comment. DataExtractor is a copy of the one from LLDB from a while back and changes have been made to adapt it to llvm. DataExtractor was designed so that you can have one of them (like for .debug_info or any other DWARF section) and use this same extractor from multiple

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath added a comment. In D63591#1553757 , @dblaikie wrote: > Given figuring out error handling for DataExtractor is perhaps a wider issue > - if you want to go ahead with this change (continue with the review & defer > error handling improvements for

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath updated this revision to Diff 206205. labath added a comment. Leave a TODO in the code. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 Files: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

dblaikie added a comment. Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine. Repository: rL LLVM CHANGES SINCE

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson added a comment. Pick whatever mechanism you like, we should debate it in that patch not here. :-) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 ___ lldb-commits

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath added a comment. In D63591#1553600 , @probinson wrote: > Ah, hadn't considered statefulness. But if you layer another class on top of > DataExtractor to handle the error flag, it would have to be replicating all > the offset-is-valid checks,

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson added a comment. Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors. I have a couple more

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath added a comment. In D63591#1553416 , @probinson wrote: > The idea for error handling for DataExtractor sounds reasonable, looks like > adding an error flag wouldn't even increase the size. Hmm... Originally I was thinking of building something

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson added a comment. Removing that llvm_unreachable is fine, in that case. The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size. Repository: rL LLVM CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:31 + +static llvm::Error createOverflowError(const char *Section) { + return createError("location list overflows the %s section", Section); dblaikie wrote: > Should this be

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath updated this revision to Diff 205973. labath marked 7 inline comments as done. labath added a comment. - remove fancy references - remove llvm_unreachable Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 Files:

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220 } - return LL; + llvm_unreachable("Exit from function inside while loop!"); } dblaikie wrote: > Given the loop condition is "while (true)" this unreachable seems a bit

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

dblaikie added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:27 +template +static llvm::Error createError(const char *Fmt, Ts &&... Vals) { + return createStringError(inconvertibleErrorCode(), Fmt, Vals...); I guess "Ts &&... Vals" should

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM but give the West Coast folks a chance to look at it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath updated this revision to Diff 205792. labath marked 3 inline comments as done. labath added a comment. - add createOverflowError helper - use unique file names in tests Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:115 -if (!Data.isValidOffsetForDataOfSize(*Offset, 2)) { - WithColor::error() << "location list overflows the debug_loc section.\n"; - return None; -} +if

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

probinson added a comment. Looks pretty good, and thanks especially for the error-case tests! I'll give other folks a chance to chime in if they want to. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:101 +if (!Data.isValidOffsetForDataOfSize(*Offset, 2 *

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

labath created this revision. labath added reviewers: dblaikie, JDevlieghere, probinson. Herald added a project: LLVM. While examining this class for possible use in lldb, I noticed two things: - it spits out parsing errors directly to stderr - the loclists parser can incorrectly return valid